* [PATCH 1/2] Add binding documentation for Zodiac Watchdog Timer @ 2015-11-18 16:38 ` Martyn Welch 0 siblings, 0 replies; 7+ messages in thread From: Martyn Welch @ 2015-11-18 16:38 UTC (permalink / raw) To: Wim Van Sebroeck Cc: linux-watchdog, Martyn Welch, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree 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> --- Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt | 12 ++++++++++++ 1 file changed, 12 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..2f34157 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt @@ -0,0 +1,12 @@ +Zodiac RAVE Watchdog Timer + +Required properties: +- compatible: must be "zii,rave-wdt" +- reg: i2c slave address of device, usually 0x38 + +Example: + + watchdog@38 { + compatible = "zii,rave-wdt"; + reg = <0x38>; + }; -- 2.1.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 1/2] Add binding documentation for Zodiac Watchdog Timer @ 2015-11-18 16:38 ` Martyn Welch 0 siblings, 0 replies; 7+ messages in thread From: Martyn Welch @ 2015-11-18 16:38 UTC (permalink / raw) To: Wim Van Sebroeck Cc: linux-watchdog-u79uwXL29TY76Z2rM5mHXA, Martyn Welch, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA 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> --- Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt | 12 ++++++++++++ 1 file changed, 12 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..2f34157 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt @@ -0,0 +1,12 @@ +Zodiac RAVE Watchdog Timer + +Required properties: +- compatible: must be "zii,rave-wdt" +- reg: i2c slave address of device, usually 0x38 + +Example: + + watchdog@38 { + compatible = "zii,rave-wdt"; + reg = <0x38>; + }; -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver 2015-11-18 16:38 ` Martyn Welch (?) @ 2015-11-18 16:38 ` Martyn Welch 2015-11-20 18:44 ` Guenter Roeck -1 siblings, 1 reply; 7+ messages in thread From: Martyn Welch @ 2015-11-18 16:38 UTC (permalink / raw) To: Wim Van Sebroeck; +Cc: linux-watchdog, Martyn Welch This patch adds a driver for the Zodiac Aerospace RAVE Watchdog Procesor. This device implements a watchdog timer, accessible over I2C. This driver implements some functionality that isn't exposed via the standard API and so the driver functionality has also been exposed via sysfs. Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk> --- drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/ziirave_wdt.c | 648 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 660 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..7577c25 --- /dev/null +++ b/drivers/watchdog/ziirave_wdt.c @@ -0,0 +1,648 @@ +/* + * 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 ziirave_wdt_rev bootloader_rev; + struct ziirave_wdt_rev firmware_rev; + int reset_reason; +}; + +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 watchdog_device *wdd, + struct ziirave_wdt_rev *rev) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + rev->part = 2; + rev->major = (char)i2c_smbus_read_byte_data(client, + ZIIRAVE_WDT_FIRM_VER_MAJOR); + + rev->minor = (char)i2c_smbus_read_byte_data(client, + ZIIRAVE_WDT_FIRM_VER_MINOR); + + return 0; +} + +static int ziirave_wdt_bootloader_rev(struct watchdog_device *wdd, + struct ziirave_wdt_rev *rev) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + rev->part = 1; + rev->major = (char)i2c_smbus_read_byte_data(client, + ZIIRAVE_WDT_BOOT_VER_MAJOR); + + rev->minor = (char)i2c_smbus_read_byte_data(client, + ZIIRAVE_WDT_BOOT_VER_MINOR); + + return 0; +} + +static int ziirave_wdt_reason(struct watchdog_device *wdd) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_RESET_REASON); +} + +static int ziirave_wdt_get_state(struct watchdog_device *wdd) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_STATE); +} + +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_get_timeout(struct watchdog_device *wdd) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIMEOUT); +} + +static int ziirave_wdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + int ret; + + if (watchdog_timeout_invalid(wdd, timeout)) + return -EINVAL; + + ret = i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_TIMEOUT, + (char)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); + + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIME_LEFT); +} + +static int ziirave_wdt_set_resettime(struct watchdog_device *wdd, + unsigned int timeout) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + if (timeout < 0 || timeout > 256) + return -EINVAL; + + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_TIMEOUT, + (char)timeout); +} + +static unsigned int ziirave_wdt_get_resettime(struct watchdog_device *wdd) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_RESET_DURATION); +} + +static int ziirave_wdt_resetwdt(struct watchdog_device *wdd) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_WDT, 0x1); +} + +static int ziirave_wdt_resetwdt_bootloader(struct watchdog_device *wdd) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_GOTO_BOOTLOADER, + 0x1); +} + +static int ziirave_wdt_resethost(struct watchdog_device *wdd) +{ + struct i2c_client *client = to_i2c_client(wdd->parent); + + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_FORCE_RESET_HOST, + 0x1); +} + +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 watchdog_device *wdd = i2c_get_clientdata(client); + struct ziirave_wdt_data *w_priv = watchdog_get_drvdata(wdd); + + 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 watchdog_device *wdd = i2c_get_clientdata(client); + struct ziirave_wdt_data *w_priv = watchdog_get_drvdata(wdd); + + 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 watchdog_device *wdd = i2c_get_clientdata(client); + struct ziirave_wdt_data *w_priv = watchdog_get_drvdata(wdd); + + 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 ssize_t ziirave_wdt_sysfs_store_state(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + int state; + + if (!strcmp(buf, "enabled") || !strcmp(buf, "enabled\n")) + state = ZIIRAVE_STATE_ON; + else if (!strcmp(buf, "disabled") || !strcmp(buf, "disabled\n")) + state = ZIIRAVE_STATE_OFF; + else + return -EINVAL; + + mutex_lock(&wdd->lock); + + ret = ziirave_wdt_set_state(wdd, state); + if (ret) { + mutex_unlock(&wdd->lock); + return -EIO; + } + + if (state == ZIIRAVE_STATE_ON) + set_bit(WDOG_ACTIVE, &wdd->status); + else + clear_bit(WDOG_ACTIVE, &wdd->status); + + mutex_unlock(&wdd->lock); + + return count; +} + +static ssize_t ziirave_wdt_sysfs_show_state(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + + ret = ziirave_wdt_get_state(wdd); + if (ret < 0) + return ret; + + if (ret >= ARRAY_SIZE(ziirave_states)) + return sprintf(buf, "Invalid"); + + return sprintf(buf, "%s", ziirave_states[ret]); +} + +static DEVICE_ATTR(state, S_IRUGO|S_IWUSR, + ziirave_wdt_sysfs_show_state, ziirave_wdt_sysfs_store_state); + +static ssize_t ziirave_wdt_sysfs_store_timeout(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + int timeout; + + ret = kstrtouint(buf, 0, &timeout); + if (ret) + return ret; + + mutex_lock(&wdd->lock); + + ret = ziirave_wdt_set_timeout(wdd, timeout); + if (ret) { + mutex_unlock(&wdd->lock); + return ret; + } + + mutex_unlock(&wdd->lock); + + return count; +} + +static ssize_t ziirave_wdt_sysfs_show_timeout(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + + ret = ziirave_wdt_get_timeout(wdd); + if (ret < 0) + return ret; + + return sprintf(buf, "%u", ret); +} + +static DEVICE_ATTR(timeout, S_IRUGO|S_IWUSR, + ziirave_wdt_sysfs_show_timeout, + ziirave_wdt_sysfs_store_timeout); + +static ssize_t ziirave_wdt_sysfs_show_timeleft(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + + ret = ziirave_wdt_get_timeout(wdd); + if (ret < 0) + return ret; + + return sprintf(buf, "%u", ret); +} + +static DEVICE_ATTR(timeleft, S_IRUGO, + ziirave_wdt_sysfs_show_timeleft, NULL); + +static ssize_t ziirave_wdt_sysfs_store_pet(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + + ret = ziirave_wdt_get_state(wdd); + if (ret != ZIIRAVE_STATE_ON) + return -EINVAL; + + ret = ziirave_wdt_ping(wdd); + if (ret) + return ret; + + return count; +} + +static DEVICE_ATTR(keepalive, S_IRUGO|S_IWUSR, NULL, + ziirave_wdt_sysfs_store_pet); + +static ssize_t ziirave_wdt_sysfs_store_reset_time(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + int reset_time; + + ret = kstrtouint(buf, 0, &reset_time); + if (ret) + return ret; + + ret = ziirave_wdt_set_resettime(wdd, reset_time); + if (ret) + return ret; + + return count; +} + +static ssize_t ziirave_wdt_sysfs_show_reset_time(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + + ret = ziirave_wdt_get_resettime(wdd); + if (ret < 0) + return ret; + + return sprintf(buf, "%u", ret); +} + +static DEVICE_ATTR(reset_duration, S_IRUGO|S_IWUSR, + ziirave_wdt_sysfs_show_reset_time, + ziirave_wdt_sysfs_store_reset_time); + +static ssize_t ziirave_wdt_sysfs_store_reset_processor(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + + if (strcmp(buf, "reset") && strcmp(buf, "reset\n")) + return -EINVAL; + + ret = ziirave_wdt_resetwdt(wdd); + if (ret) + return ret; + + return count; +} + +static DEVICE_ATTR(reset_wdt, S_IRUGO|S_IWUSR, NULL, + ziirave_wdt_sysfs_store_reset_processor); + +static ssize_t ziirave_wdt_sysfs_store_wdt_to_bootloader( + struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + + if (strcmp(buf, "reset") && strcmp(buf, "reset\n")) + return -EINVAL; + + ret = ziirave_wdt_resetwdt_bootloader(wdd); + if (ret) + return ret; + + return count; +} + +static DEVICE_ATTR(reset_bootloader, S_IRUGO|S_IWUSR, NULL, + ziirave_wdt_sysfs_store_wdt_to_bootloader); + +static ssize_t ziirave_wdt_sysfs_store_reset_host( + struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct i2c_client *client = to_i2c_client(dev->parent); + struct watchdog_device *wdd = i2c_get_clientdata(client); + int ret; + + if (strcmp(buf, "reset") && strcmp(buf, "reset\n")) + return -EINVAL; + + ret = ziirave_wdt_resethost(wdd); + if (ret) + return ret; + + return count; +} + +static DEVICE_ATTR(force_reset, S_IRUGO|S_IWUSR, NULL, + ziirave_wdt_sysfs_store_reset_host); + +static struct attribute *ziirave_wdt_attrs[] = { + &dev_attr_firmware_version.attr, + &dev_attr_bootloader_version.attr, + &dev_attr_reset_reason.attr, + &dev_attr_state.attr, + &dev_attr_timeout.attr, + &dev_attr_timeleft.attr, + &dev_attr_keepalive.attr, + &dev_attr_reset_duration.attr, + &dev_attr_reset_wdt.attr, + &dev_attr_reset_bootloader.attr, + &dev_attr_force_reset.attr, + NULL +}; + +static const struct attribute_group ziirave_wdt_sysfs_files = { + .attrs = ziirave_wdt_attrs, +}; + +static int ziirave_wdt_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + int ret = 0; + struct watchdog_device *wdd; + struct ziirave_wdt_data *w_priv; + + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C + | I2C_FUNC_SMBUS_BYTE_DATA + | I2C_FUNC_SMBUS_READ_I2C_BLOCK)) + return -ENODEV; + + wdd = devm_kzalloc(&client->dev, sizeof(*wdd), GFP_KERNEL); + if (!wdd) + return -ENOMEM; + + w_priv = devm_kzalloc(&client->dev, sizeof(*w_priv), GFP_KERNEL); + if (!wdd) + return -ENOMEM; + + wdd->info = &ziirave_wdt_info; + wdd->ops = &ziirave_wdt_ops; + wdd->status = 0; + wdd->timeout = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIMEOUT); + wdd->min_timeout = ZIIRAVE_TIMEOUT_MIN; + wdd->max_timeout = ZIIRAVE_TIMEOUT_MAX; + wdd->parent = &client->dev; + + watchdog_set_nowayout(wdd, nowayout); + + i2c_set_clientdata(client, wdd); + + ziirave_wdt_stop(wdd); + + ret = watchdog_register_device(wdd); + if (ret) + return ret; + + ret = ziirave_wdt_firmware_rev(wdd, &w_priv->firmware_rev); + if (ret) + goto err; + + ret = ziirave_wdt_bootloader_rev(wdd, &w_priv->bootloader_rev); + if (ret) + goto err; + + w_priv->reset_reason = ziirave_wdt_reason(wdd); + if ((ret < 0) || (ret >= ARRAY_SIZE(ziirave_reasons))) + goto err; + + watchdog_set_drvdata(wdd, w_priv); + + ret = sysfs_create_group(&wdd->dev->kobj, &ziirave_wdt_sysfs_files); + if (ret) + goto err; + + return 0; +err: + watchdog_unregister_device(wdd); + + return ret; +} + +static int ziirave_wdt_remove(struct i2c_client *client) +{ + struct watchdog_device *wdd = i2c_get_clientdata(client); + + sysfs_remove_group(&client->dev.kobj, &ziirave_wdt_sysfs_files); + + watchdog_unregister_device(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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver 2015-11-18 16:38 ` [PATCH 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver Martyn Welch @ 2015-11-20 18:44 ` Guenter Roeck 2015-11-23 11:25 ` Martyn Welch 0 siblings, 1 reply; 7+ messages in thread From: Guenter Roeck @ 2015-11-20 18:44 UTC (permalink / raw) To: Martyn Welch, Wim Van Sebroeck; +Cc: linux-watchdog On 11/18/2015 08:38 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. > > This driver implements some functionality that isn't exposed via the > standard API and so the driver functionality has also been exposed via > sysfs. > Not a complete review, just a start. This will require some major changes to be acceptable. > Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk> > --- > drivers/watchdog/Kconfig | 11 + > drivers/watchdog/Makefile | 1 + > drivers/watchdog/ziirave_wdt.c | 648 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 660 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..7577c25 > --- /dev/null > +++ b/drivers/watchdog/ziirave_wdt.c > @@ -0,0 +1,648 @@ > +/* > + * 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 ziirave_wdt_rev bootloader_rev; > + struct ziirave_wdt_rev firmware_rev; > + int reset_reason; > +}; > + > +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 watchdog_device *wdd, > + struct ziirave_wdt_rev *rev) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + rev->part = 2; > + rev->major = (char)i2c_smbus_read_byte_data(client, > + ZIIRAVE_WDT_FIRM_VER_MAJOR); > + > + rev->minor = (char)i2c_smbus_read_byte_data(client, > + ZIIRAVE_WDT_FIRM_VER_MINOR); > + I am not really a friend of ignoring errors. With the typecast, errors will translate to an odd version code. Besides, you defined 'minor' and 'major' as unsigned char, and typecast to char, meaning the values written into the variables will be really weird on errors (eg 251 for -EIO). > + return 0; A static function always returning 0 doesn't really make any sense. > +} > + > +static int ziirave_wdt_bootloader_rev(struct watchdog_device *wdd, > + struct ziirave_wdt_rev *rev) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + rev->part = 1; > + rev->major = (char)i2c_smbus_read_byte_data(client, > + ZIIRAVE_WDT_BOOT_VER_MAJOR); > + > + rev->minor = (char)i2c_smbus_read_byte_data(client, > + ZIIRAVE_WDT_BOOT_VER_MINOR); > + > + return 0; > +} > + > +static int ziirave_wdt_reason(struct watchdog_device *wdd) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_RESET_REASON); I think it would be better to store client in struct ziirave_wdt_data and access it from there. I am kind of ambivalent on the access functions > +} > + > +static int ziirave_wdt_get_state(struct watchdog_device *wdd) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_STATE); > +} > + > +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); Please align continuation lines with '('. > +} > + > +static int ziirave_wdt_get_timeout(struct watchdog_device *wdd) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIMEOUT); > +} > + > +static int ziirave_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + int ret; > + > + if (watchdog_timeout_invalid(wdd, timeout)) > + return -EINVAL; > + Unnecessary; handled by infrastructure (and a sysfs attribute to set it is unacceptable outside the infrastructure - please see below). > + ret = i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_TIMEOUT, > + (char)timeout); Unnecessary (and wrong) typecast. > + 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); > + > + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIME_LEFT); While it is unfortunate that get_timeleft does not support returning an error code, converting it into large numbers is even less desirable. > +} > + > +static int ziirave_wdt_set_resettime(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + if (timeout < 0 || timeout > 256) > + return -EINVAL; Wrong range check. > + > + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_TIMEOUT, > + (char)timeout); char conversion is odd here. 256 -> 0 ? 255 -> -1 ? The typecast should be unnecessary. Also, I am a bit lost here. Isn't this the same as the timeout set above ? > +} > + > +static unsigned int ziirave_wdt_get_resettime(struct watchdog_device *wdd) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_RESET_DURATION); Ignoring an error return code here is not a good idea. -EIO will translate to a really large number (something like 4294967291). > +} > + > +static int ziirave_wdt_resetwdt(struct watchdog_device *wdd) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_WDT, 0x1); > +} > + > +static int ziirave_wdt_resetwdt_bootloader(struct watchdog_device *wdd) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_GOTO_BOOTLOADER, > + 0x1); > +} > + > +static int ziirave_wdt_resethost(struct watchdog_device *wdd) > +{ > + struct i2c_client *client = to_i2c_client(wdd->parent); > + > + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_FORCE_RESET_HOST, > + 0x1); > +} > + > +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 watchdog_device *wdd = i2c_get_clientdata(client); > + struct ziirave_wdt_data *w_priv = watchdog_get_drvdata(wdd); > + > + 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 watchdog_device *wdd = i2c_get_clientdata(client); > + struct ziirave_wdt_data *w_priv = watchdog_get_drvdata(wdd); > + > + 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 watchdog_device *wdd = i2c_get_clientdata(client); > + struct ziirave_wdt_data *w_priv = watchdog_get_drvdata(wdd); > + > + if ((w_priv->reset_reason < 0) || > + (w_priv->reset_reason >= ARRAY_SIZE(ziirave_states))) Please no unnecessary ( ). > + 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 ssize_t ziirave_wdt_sysfs_store_state(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + int state; > + > + if (!strcmp(buf, "enabled") || !strcmp(buf, "enabled\n")) > + state = ZIIRAVE_STATE_ON; > + else if (!strcmp(buf, "disabled") || !strcmp(buf, "disabled\n")) > + state = ZIIRAVE_STATE_OFF; > + else > + return -EINVAL; > + > + mutex_lock(&wdd->lock); > + > + ret = ziirave_wdt_set_state(wdd, state); > + if (ret) { > + mutex_unlock(&wdd->lock); > + return -EIO; Please use goto here, and don't overwrite errors from called code. Mostly irrelevant, though, since the attribute itself is not acceptable. > + } > + > + if (state == ZIIRAVE_STATE_ON) > + set_bit(WDOG_ACTIVE, &wdd->status); > + else > + clear_bit(WDOG_ACTIVE, &wdd->status); That is not the correct way to enable a watchdog. Please use the infrastructure. > + > + mutex_unlock(&wdd->lock); > + > + return count; > +} > + > +static ssize_t ziirave_wdt_sysfs_show_state(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + > + ret = ziirave_wdt_get_state(wdd); > + if (ret < 0) > + return ret; > + > + if (ret >= ARRAY_SIZE(ziirave_states)) > + return sprintf(buf, "Invalid"); > + > + return sprintf(buf, "%s", ziirave_states[ret]); > +} > + > +static DEVICE_ATTR(state, S_IRUGO|S_IWUSR, > + ziirave_wdt_sysfs_show_state, ziirave_wdt_sysfs_store_state); > + > +static ssize_t ziirave_wdt_sysfs_store_timeout(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + int timeout; > + > + ret = kstrtouint(buf, 0, &timeout); > + if (ret) > + return ret; > + > + mutex_lock(&wdd->lock); > + > + ret = ziirave_wdt_set_timeout(wdd, timeout); > + if (ret) { > + mutex_unlock(&wdd->lock); > + return ret; > + } > + > + mutex_unlock(&wdd->lock); > + > + return count; > +} > + > +static ssize_t ziirave_wdt_sysfs_show_timeout(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + > + ret = ziirave_wdt_get_timeout(wdd); > + if (ret < 0) > + return ret; > + > + return sprintf(buf, "%u", ret); > +} > + > +static DEVICE_ATTR(timeout, S_IRUGO|S_IWUSR, > + ziirave_wdt_sysfs_show_timeout, > + ziirave_wdt_sysfs_store_timeout); > + > +static ssize_t ziirave_wdt_sysfs_show_timeleft(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + > + ret = ziirave_wdt_get_timeout(wdd); > + if (ret < 0) > + return ret; > + > + return sprintf(buf, "%u", ret); > +} > + > +static DEVICE_ATTR(timeleft, S_IRUGO, > + ziirave_wdt_sysfs_show_timeleft, NULL); > + Please drop all those sysfs attributes which are supposed to be handled by the infrastructure. If anything, please help getting the respective infrastructure changes upstream to make those attributes visible as syfs attributes. I can point you to the patch if needed. > +static ssize_t ziirave_wdt_sysfs_store_pet(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + > + ret = ziirave_wdt_get_state(wdd); > + if (ret != ZIIRAVE_STATE_ON) > + return -EINVAL; > + > + ret = ziirave_wdt_ping(wdd); > + if (ret) > + return ret; > + > + return count; > +} > + > +static DEVICE_ATTR(keepalive, S_IRUGO|S_IWUSR, NULL, > + ziirave_wdt_sysfs_store_pet); > + > +static ssize_t ziirave_wdt_sysfs_store_reset_time(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + int reset_time; > + > + ret = kstrtouint(buf, 0, &reset_time); > + if (ret) > + return ret; > + > + ret = ziirave_wdt_set_resettime(wdd, reset_time); > + if (ret) > + return ret; > + > + return count; > +} > + > +static ssize_t ziirave_wdt_sysfs_show_reset_time(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + > + ret = ziirave_wdt_get_resettime(wdd); > + if (ret < 0) > + return ret; > + > + return sprintf(buf, "%u", ret); > +} > + > +static DEVICE_ATTR(reset_duration, S_IRUGO|S_IWUSR, > + ziirave_wdt_sysfs_show_reset_time, > + ziirave_wdt_sysfs_store_reset_time); > + > +static ssize_t ziirave_wdt_sysfs_store_reset_processor(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + > + if (strcmp(buf, "reset") && strcmp(buf, "reset\n")) > + return -EINVAL; > + > + ret = ziirave_wdt_resetwdt(wdd); Is this supposed to cause a system reset ? If so, wrong way to do it. Please register a reset notifier. > + if (ret) > + return ret; > + > + return count; > +} > + > +static DEVICE_ATTR(reset_wdt, S_IRUGO|S_IWUSR, NULL, > + ziirave_wdt_sysfs_store_reset_processor); > + > +static ssize_t ziirave_wdt_sysfs_store_wdt_to_bootloader( > + struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + > + if (strcmp(buf, "reset") && strcmp(buf, "reset\n")) > + return -EINVAL; > + > + ret = ziirave_wdt_resetwdt_bootloader(wdd); > + if (ret) > + return ret; > + > + return count; > +} > + > +static DEVICE_ATTR(reset_bootloader, S_IRUGO|S_IWUSR, NULL, > + ziirave_wdt_sysfs_store_wdt_to_bootloader); > + > +static ssize_t ziirave_wdt_sysfs_store_reset_host( > + struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + int ret; > + > + if (strcmp(buf, "reset") && strcmp(buf, "reset\n")) > + return -EINVAL; > + > + ret = ziirave_wdt_resethost(wdd); > + if (ret) > + return ret; > + > + return count; > +} > + > +static DEVICE_ATTR(force_reset, S_IRUGO|S_IWUSR, NULL, > + ziirave_wdt_sysfs_store_reset_host); > + > +static struct attribute *ziirave_wdt_attrs[] = { > + &dev_attr_firmware_version.attr, > + &dev_attr_bootloader_version.attr, > + &dev_attr_reset_reason.attr, > + &dev_attr_state.attr, > + &dev_attr_timeout.attr, > + &dev_attr_timeleft.attr, > + &dev_attr_keepalive.attr, > + &dev_attr_reset_duration.attr, > + &dev_attr_reset_wdt.attr, > + &dev_attr_reset_bootloader.attr, > + &dev_attr_force_reset.attr, > + NULL > +}; > + > +static const struct attribute_group ziirave_wdt_sysfs_files = { > + .attrs = ziirave_wdt_attrs, > +}; > + > +static int ziirave_wdt_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + int ret = 0; > + struct watchdog_device *wdd; > + struct ziirave_wdt_data *w_priv; > + > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C > + | I2C_FUNC_SMBUS_BYTE_DATA > + | I2C_FUNC_SMBUS_READ_I2C_BLOCK)) > + return -ENODEV; > + > + wdd = devm_kzalloc(&client->dev, sizeof(*wdd), GFP_KERNEL); > + if (!wdd) > + return -ENOMEM; > + > + w_priv = devm_kzalloc(&client->dev, sizeof(*w_priv), GFP_KERNEL); > + if (!wdd) > + return -ENOMEM; > + Please allocate wdd as part of w_priv; two separate allocations are not needed. > + wdd->info = &ziirave_wdt_info; > + wdd->ops = &ziirave_wdt_ops; > + wdd->status = 0; > + wdd->timeout = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIMEOUT); No error check ? > + wdd->min_timeout = ZIIRAVE_TIMEOUT_MIN; > + wdd->max_timeout = ZIIRAVE_TIMEOUT_MAX; > + wdd->parent = &client->dev; > + > + watchdog_set_nowayout(wdd, nowayout); > + Since the driver supports devicetree, it may make sense to also call watchdog_set_timeout() to pick up a timeout configured in devicetree data. > + i2c_set_clientdata(client, wdd); > + > + ziirave_wdt_stop(wdd); Are you sure ? This will leave the system unprotected until user space starts. > + > + ret = watchdog_register_device(wdd); > + if (ret) > + return ret; > + > + ret = ziirave_wdt_firmware_rev(wdd, &w_priv->firmware_rev); > + if (ret) > + goto err; > + > + ret = ziirave_wdt_bootloader_rev(wdd, &w_priv->bootloader_rev); > + if (ret) > + goto err; > + But those functions always return 0. > + w_priv->reset_reason = ziirave_wdt_reason(wdd); > + if ((ret < 0) || (ret >= ARRAY_SIZE(ziirave_reasons))) Unnecessary ( ). And is this really fatal ? > + goto err; > + > + watchdog_set_drvdata(wdd, w_priv); > + > + ret = sysfs_create_group(&wdd->dev->kobj, &ziirave_wdt_sysfs_files); > + if (ret) > + goto err; > + > + return 0; > +err: > + watchdog_unregister_device(wdd); > + > + return ret; > +} > + > +static int ziirave_wdt_remove(struct i2c_client *client) > +{ > + struct watchdog_device *wdd = i2c_get_clientdata(client); > + > + sysfs_remove_group(&client->dev.kobj, &ziirave_wdt_sysfs_files); > + > + watchdog_unregister_device(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"); > + > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver 2015-11-20 18:44 ` Guenter Roeck @ 2015-11-23 11:25 ` Martyn Welch 0 siblings, 0 replies; 7+ messages in thread From: Martyn Welch @ 2015-11-23 11:25 UTC (permalink / raw) To: Guenter Roeck, Wim Van Sebroeck; +Cc: linux-watchdog On 20/11/15 18:44, Guenter Roeck wrote: > On 11/18/2015 08:38 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. >> >> This driver implements some functionality that isn't exposed via the >> standard API and so the driver functionality has also been exposed via >> sysfs. >> > > Not a complete review, just a start. This will require some major changes > to be acceptable. > No problem, thanks for the feedback, comments below. Martyn >> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk> >> --- >> drivers/watchdog/Kconfig | 11 + >> drivers/watchdog/Makefile | 1 + >> drivers/watchdog/ziirave_wdt.c | 648 >> +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 660 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..7577c25 >> --- /dev/null >> +++ b/drivers/watchdog/ziirave_wdt.c >> @@ -0,0 +1,648 @@ >> +/* >> + * 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 ziirave_wdt_rev bootloader_rev; >> + struct ziirave_wdt_rev firmware_rev; >> + int reset_reason; >> +}; >> + >> +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 watchdog_device *wdd, >> + struct ziirave_wdt_rev *rev) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + rev->part = 2; >> + rev->major = (char)i2c_smbus_read_byte_data(client, >> + ZIIRAVE_WDT_FIRM_VER_MAJOR); >> + >> + rev->minor = (char)i2c_smbus_read_byte_data(client, >> + ZIIRAVE_WDT_FIRM_VER_MINOR); >> + > I am not really a friend of ignoring errors. With the typecast, errors > will translate to an odd version code. Besides, you defined 'minor' > and 'major' as unsigned char, and typecast to char, meaning the values > written into the variables will be really weird on errors (eg 251 > for -EIO). > OK, will fix. >> + return 0; > > A static function always returning 0 doesn't really make any sense. > I guess this will be resolved once I'm checking the return values... >> +} >> + >> +static int ziirave_wdt_bootloader_rev(struct watchdog_device *wdd, >> + struct ziirave_wdt_rev *rev) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + rev->part = 1; >> + rev->major = (char)i2c_smbus_read_byte_data(client, >> + ZIIRAVE_WDT_BOOT_VER_MAJOR); >> + >> + rev->minor = (char)i2c_smbus_read_byte_data(client, >> + ZIIRAVE_WDT_BOOT_VER_MINOR); >> + >> + return 0; >> +} >> + >> +static int ziirave_wdt_reason(struct watchdog_device *wdd) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_RESET_REASON); > > I think it would be better to store client in struct ziirave_wdt_data > and access it from there. I am kind of ambivalent on the access functions > Will take a look. > >> +} >> + >> +static int ziirave_wdt_get_state(struct watchdog_device *wdd) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_STATE); >> +} >> + >> +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); > > Please align continuation lines with '('. > Will do. >> +} >> + >> +static int ziirave_wdt_get_timeout(struct watchdog_device *wdd) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIMEOUT); >> +} >> + >> +static int ziirave_wdt_set_timeout(struct watchdog_device *wdd, >> + unsigned int timeout) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + int ret; >> + >> + if (watchdog_timeout_invalid(wdd, timeout)) >> + return -EINVAL; >> + > Unnecessary; handled by infrastructure (and a sysfs attribute to set it > is unacceptable outside the infrastructure - please see below). > OK, will remove. >> + ret = i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_TIMEOUT, >> + (char)timeout); > > Unnecessary (and wrong) typecast. 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); >> + >> + return i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIME_LEFT); > > While it is unfortunate that get_timeleft does not support returning > an error code, > converting it into large numbers is even less desirable. > Error handling. OK. >> +} >> + >> +static int ziirave_wdt_set_resettime(struct watchdog_device *wdd, >> + unsigned int timeout) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + if (timeout < 0 || timeout > 256) >> + return -EINVAL; > > Wrong range check. > Doh. >> + >> + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_TIMEOUT, >> + (char)timeout); > > char conversion is odd here. 256 -> 0 ? 255 -> -1 ? > The typecast should be unnecessary. > > Also, I am a bit lost here. Isn't this the same as the timeout set > above ? > This is the length of time that the device holds the reset pin high when the watchdog is triggered. Timeout is the wrong name for this variable, I'll change it. >> +} >> + >> +static unsigned int ziirave_wdt_get_resettime(struct watchdog_device >> *wdd) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + return i2c_smbus_read_byte_data(client, >> ZIIRAVE_WDT_RESET_DURATION); > > Ignoring an error return code here is not a good idea. -EIO will > translate > to a really large number (something like 4294967291). > OK. >> +} >> + >> +static int ziirave_wdt_resetwdt(struct watchdog_device *wdd) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_WDT, >> 0x1); >> +} >> + >> +static int ziirave_wdt_resetwdt_bootloader(struct watchdog_device *wdd) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + return i2c_smbus_write_byte_data(client, >> ZIIRAVE_WDT_GOTO_BOOTLOADER, >> + 0x1); >> +} >> + >> +static int ziirave_wdt_resethost(struct watchdog_device *wdd) >> +{ >> + struct i2c_client *client = to_i2c_client(wdd->parent); >> + >> + return i2c_smbus_write_byte_data(client, >> ZIIRAVE_WDT_FORCE_RESET_HOST, >> + 0x1); >> +} >> + >> +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 watchdog_device *wdd = i2c_get_clientdata(client); >> + struct ziirave_wdt_data *w_priv = watchdog_get_drvdata(wdd); >> + >> + 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 watchdog_device *wdd = i2c_get_clientdata(client); >> + struct ziirave_wdt_data *w_priv = watchdog_get_drvdata(wdd); >> + >> + 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 watchdog_device *wdd = i2c_get_clientdata(client); >> + struct ziirave_wdt_data *w_priv = watchdog_get_drvdata(wdd); >> + >> + if ((w_priv->reset_reason < 0) || >> + (w_priv->reset_reason >= ARRAY_SIZE(ziirave_states))) > > Please no unnecessary ( ). > OK >> + 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 ssize_t ziirave_wdt_sysfs_store_state(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + int state; >> + >> + if (!strcmp(buf, "enabled") || !strcmp(buf, "enabled\n")) >> + state = ZIIRAVE_STATE_ON; >> + else if (!strcmp(buf, "disabled") || !strcmp(buf, "disabled\n")) >> + state = ZIIRAVE_STATE_OFF; >> + else >> + return -EINVAL; >> + >> + mutex_lock(&wdd->lock); >> + >> + ret = ziirave_wdt_set_state(wdd, state); >> + if (ret) { >> + mutex_unlock(&wdd->lock); >> + return -EIO; > > Please use goto here, and don't overwrite errors from called code. > Mostly irrelevant, though, since the attribute itself is not acceptable. > Ok >> + } >> + >> + if (state == ZIIRAVE_STATE_ON) >> + set_bit(WDOG_ACTIVE, &wdd->status); >> + else >> + clear_bit(WDOG_ACTIVE, &wdd->status); > > That is not the correct way to enable a watchdog. Please use the > infrastructure. > I'll remove this sysfs entry then. >> + >> + mutex_unlock(&wdd->lock); >> + >> + return count; >> +} >> + >> +static ssize_t ziirave_wdt_sysfs_show_state(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + >> + ret = ziirave_wdt_get_state(wdd); >> + if (ret < 0) >> + return ret; >> + >> + if (ret >= ARRAY_SIZE(ziirave_states)) >> + return sprintf(buf, "Invalid"); >> + >> + return sprintf(buf, "%s", ziirave_states[ret]); >> +} >> + >> +static DEVICE_ATTR(state, S_IRUGO|S_IWUSR, >> + ziirave_wdt_sysfs_show_state, ziirave_wdt_sysfs_store_state); >> + >> +static ssize_t ziirave_wdt_sysfs_store_timeout(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + int timeout; >> + >> + ret = kstrtouint(buf, 0, &timeout); >> + if (ret) >> + return ret; >> + >> + mutex_lock(&wdd->lock); >> + >> + ret = ziirave_wdt_set_timeout(wdd, timeout); >> + if (ret) { >> + mutex_unlock(&wdd->lock); >> + return ret; >> + } >> + >> + mutex_unlock(&wdd->lock); >> + >> + return count; >> +} >> + >> +static ssize_t ziirave_wdt_sysfs_show_timeout(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + >> + ret = ziirave_wdt_get_timeout(wdd); >> + if (ret < 0) >> + return ret; >> + >> + return sprintf(buf, "%u", ret); >> +} >> + >> +static DEVICE_ATTR(timeout, S_IRUGO|S_IWUSR, >> + ziirave_wdt_sysfs_show_timeout, >> + ziirave_wdt_sysfs_store_timeout); >> + >> +static ssize_t ziirave_wdt_sysfs_show_timeleft(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + >> + ret = ziirave_wdt_get_timeout(wdd); >> + if (ret < 0) >> + return ret; >> + >> + return sprintf(buf, "%u", ret); >> +} >> + >> +static DEVICE_ATTR(timeleft, S_IRUGO, >> + ziirave_wdt_sysfs_show_timeleft, NULL); >> + > > Please drop all those sysfs attributes which are supposed to be handled > by the infrastructure. Will do. > If anything, please help getting the respective > infrastructure changes upstream to make those attributes visible as > syfs attributes. I can point you to the patch if needed. > >> +static ssize_t ziirave_wdt_sysfs_store_pet(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + >> + ret = ziirave_wdt_get_state(wdd); >> + if (ret != ZIIRAVE_STATE_ON) >> + return -EINVAL; >> + >> + ret = ziirave_wdt_ping(wdd); >> + if (ret) >> + return ret; >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR(keepalive, S_IRUGO|S_IWUSR, NULL, >> + ziirave_wdt_sysfs_store_pet); >> + >> +static ssize_t ziirave_wdt_sysfs_store_reset_time(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + int reset_time; >> + >> + ret = kstrtouint(buf, 0, &reset_time); >> + if (ret) >> + return ret; >> + >> + ret = ziirave_wdt_set_resettime(wdd, reset_time); >> + if (ret) >> + return ret; >> + >> + return count; >> +} >> + >> +static ssize_t ziirave_wdt_sysfs_show_reset_time(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + >> + ret = ziirave_wdt_get_resettime(wdd); >> + if (ret < 0) >> + return ret; >> + >> + return sprintf(buf, "%u", ret); >> +} >> + >> +static DEVICE_ATTR(reset_duration, S_IRUGO|S_IWUSR, >> + ziirave_wdt_sysfs_show_reset_time, >> + ziirave_wdt_sysfs_store_reset_time); >> + >> +static ssize_t ziirave_wdt_sysfs_store_reset_processor(struct device >> *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + >> + if (strcmp(buf, "reset") && strcmp(buf, "reset\n")) >> + return -EINVAL; >> + >> + ret = ziirave_wdt_resetwdt(wdd); > > Is this supposed to cause a system reset ? If so, wrong way to do it. > Please register a reset notifier. > Writing to this register resets the watchdog (it's implemented on a microcontroller), not the system. >> + if (ret) >> + return ret; >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR(reset_wdt, S_IRUGO|S_IWUSR, NULL, >> + ziirave_wdt_sysfs_store_reset_processor); >> + >> +static ssize_t ziirave_wdt_sysfs_store_wdt_to_bootloader( >> + struct device *dev, struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + >> + if (strcmp(buf, "reset") && strcmp(buf, "reset\n")) >> + return -EINVAL; >> + >> + ret = ziirave_wdt_resetwdt_bootloader(wdd); >> + if (ret) >> + return ret; >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR(reset_bootloader, S_IRUGO|S_IWUSR, NULL, >> + ziirave_wdt_sysfs_store_wdt_to_bootloader); >> + >> +static ssize_t ziirave_wdt_sysfs_store_reset_host( >> + struct device *dev, struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct i2c_client *client = to_i2c_client(dev->parent); >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + int ret; >> + >> + if (strcmp(buf, "reset") && strcmp(buf, "reset\n")) >> + return -EINVAL; >> + >> + ret = ziirave_wdt_resethost(wdd); >> + if (ret) >> + return ret; >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR(force_reset, S_IRUGO|S_IWUSR, NULL, >> + ziirave_wdt_sysfs_store_reset_host); >> + >> +static struct attribute *ziirave_wdt_attrs[] = { >> + &dev_attr_firmware_version.attr, >> + &dev_attr_bootloader_version.attr, >> + &dev_attr_reset_reason.attr, >> + &dev_attr_state.attr, >> + &dev_attr_timeout.attr, >> + &dev_attr_timeleft.attr, >> + &dev_attr_keepalive.attr, >> + &dev_attr_reset_duration.attr, >> + &dev_attr_reset_wdt.attr, >> + &dev_attr_reset_bootloader.attr, >> + &dev_attr_force_reset.attr, >> + NULL >> +}; >> + >> +static const struct attribute_group ziirave_wdt_sysfs_files = { >> + .attrs = ziirave_wdt_attrs, >> +}; >> + >> +static int ziirave_wdt_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + int ret = 0; >> + struct watchdog_device *wdd; >> + struct ziirave_wdt_data *w_priv; >> + >> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C >> + | I2C_FUNC_SMBUS_BYTE_DATA >> + | I2C_FUNC_SMBUS_READ_I2C_BLOCK)) >> + return -ENODEV; >> + >> + wdd = devm_kzalloc(&client->dev, sizeof(*wdd), GFP_KERNEL); >> + if (!wdd) >> + return -ENOMEM; >> + >> + w_priv = devm_kzalloc(&client->dev, sizeof(*w_priv), GFP_KERNEL); >> + if (!wdd) >> + return -ENOMEM; >> + > > Please allocate wdd as part of w_priv; two separate allocations are > not needed. > Ok. >> + wdd->info = &ziirave_wdt_info; >> + wdd->ops = &ziirave_wdt_ops; >> + wdd->status = 0; >> + wdd->timeout = i2c_smbus_read_byte_data(client, >> ZIIRAVE_WDT_TIMEOUT); > > No error check ? OK > >> + wdd->min_timeout = ZIIRAVE_TIMEOUT_MIN; >> + wdd->max_timeout = ZIIRAVE_TIMEOUT_MAX; >> + wdd->parent = &client->dev; >> + >> + watchdog_set_nowayout(wdd, nowayout); >> + > Since the driver supports devicetree, it may make sense to also call > watchdog_set_timeout() to pick up a timeout configured in devicetree > data. > True. >> + i2c_set_clientdata(client, wdd); >> + >> + ziirave_wdt_stop(wdd); > > Are you sure ? This will leave the system unprotected until user space > starts. > This is a necessary step in the initialisation of the device. It's in an unconfigured state until that call and not running. >> + >> + ret = watchdog_register_device(wdd); >> + if (ret) >> + return ret; >> + >> + ret = ziirave_wdt_firmware_rev(wdd, &w_priv->firmware_rev); >> + if (ret) >> + goto err; >> + >> + ret = ziirave_wdt_bootloader_rev(wdd, &w_priv->bootloader_rev); >> + if (ret) >> + goto err; >> + > But those functions always return 0. Will fix the return values :-) > >> + w_priv->reset_reason = ziirave_wdt_reason(wdd); >> + if ((ret < 0) || (ret >= ARRAY_SIZE(ziirave_reasons))) > > Unnecessary ( ). And is this really fatal ? It would point to the driver not being able to properly understand the responses it's getting back from the device. Which would potentially suggest that the driver is attempting to bind against something that isn't actually the watchdog it thinks it's binding against. > >> + goto err; >> + >> + watchdog_set_drvdata(wdd, w_priv); >> + >> + ret = sysfs_create_group(&wdd->dev->kobj, >> &ziirave_wdt_sysfs_files); >> + if (ret) >> + goto err; >> + >> + return 0; >> +err: >> + watchdog_unregister_device(wdd); >> + >> + return ret; >> +} >> + >> +static int ziirave_wdt_remove(struct i2c_client *client) >> +{ >> + struct watchdog_device *wdd = i2c_get_clientdata(client); >> + >> + sysfs_remove_group(&client->dev.kobj, &ziirave_wdt_sysfs_files); >> + >> + watchdog_unregister_device(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"); >> + >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Add binding documentation for Zodiac Watchdog Timer @ 2015-11-18 21:19 ` Rob Herring 0 siblings, 0 replies; 7+ messages in thread From: Rob Herring @ 2015-11-18 21:19 UTC (permalink / raw) To: Martyn Welch Cc: Wim Van Sebroeck, linux-watchdog, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree On Wed, Nov 18, 2015 at 04:38:50PM +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> You could document this under trivial-devices.txt, but this is fine too. Acked-by: Rob Herring <robh@kernel.org> > --- > Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt | 12 ++++++++++++ > 1 file changed, 12 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..2f34157 > --- /dev/null > +++ b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt > @@ -0,0 +1,12 @@ > +Zodiac RAVE Watchdog Timer > + > +Required properties: > +- compatible: must be "zii,rave-wdt" > +- reg: i2c slave address of device, usually 0x38 > + > +Example: > + > + watchdog@38 { > + compatible = "zii,rave-wdt"; > + reg = <0x38>; > + }; > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Add binding documentation for Zodiac Watchdog Timer @ 2015-11-18 21:19 ` Rob Herring 0 siblings, 0 replies; 7+ messages in thread From: Rob Herring @ 2015-11-18 21:19 UTC (permalink / raw) To: Martyn Welch Cc: Wim Van Sebroeck, linux-watchdog-u79uwXL29TY76Z2rM5mHXA, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA On Wed, Nov 18, 2015 at 04:38:50PM +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> You could document this under trivial-devices.txt, but this is fine too. Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > --- > Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt | 12 ++++++++++++ > 1 file changed, 12 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..2f34157 > --- /dev/null > +++ b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt > @@ -0,0 +1,12 @@ > +Zodiac RAVE Watchdog Timer > + > +Required properties: > +- compatible: must be "zii,rave-wdt" > +- reg: i2c slave address of device, usually 0x38 > + > +Example: > + > + watchdog@38 { > + compatible = "zii,rave-wdt"; > + reg = <0x38>; > + }; > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-23 11:25 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-18 16:38 [PATCH 1/2] Add binding documentation for Zodiac Watchdog Timer Martyn Welch 2015-11-18 16:38 ` Martyn Welch 2015-11-18 16:38 ` [PATCH 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver Martyn Welch 2015-11-20 18:44 ` Guenter Roeck 2015-11-23 11:25 ` Martyn Welch 2015-11-18 21:19 ` [PATCH 1/2] Add binding documentation for Zodiac Watchdog Timer Rob Herring 2015-11-18 21:19 ` Rob Herring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.