* [PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver
@ 2020-06-22 10:03 Johnson CH Chen (陳昭勳)
2020-06-22 14:26 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-06-22 10:03 UTC (permalink / raw)
To: linux-kernel
Cc: linux-rtc, linux-watchdog, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, linux, Lee Jones
Here provide Watchdog function from rtc-ds1374.c which is in RTC subsystem
originally. Besides, add nowayout and implement Watchdog margin time when
DS1374 Watchdog device is found.
Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
---
drivers/watchdog/Kconfig | 11 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/ds1374_wdt.c | 330 ++++++++++++++++++++++++++++++++++
3 files changed, 342 insertions(+)
create mode 100644 drivers/watchdog/ds1374_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b739c476955b..b4ff4b3c18da 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -316,6 +316,17 @@ config ZIIRAVE_WATCHDOG
To compile this driver as a module, choose M here: the
module will be called ziirave_wdt.
+config DS1374_WATCHDOG
+ tristate "Dallas/Maxim DS1374 Watchdog timer"
+ depends on MFD_DS1374
+ select WATCHDOG_CORE
+ help
+ If you say Y here you will get support for the watchdog timer
+ in the Dallas/Maxim Semiconductor DS1374 real-time clock chips.
+
+ This driver can also be built as a module. If so the module
+ will be called ds1374_wdt.
+
config RAVE_SP_WATCHDOG
tristate "RAVE SP Watchdog timer"
depends on RAVE_SP_CORE
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 6de2e4ceef19..1c468f5d9e5f 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -224,3 +224,4 @@ obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
+obj-$(CONFIG_DS1374_WATCHDOG) += ds1374_wdt.o
diff --git a/drivers/watchdog/ds1374_wdt.c b/drivers/watchdog/ds1374_wdt.c
new file mode 100644
index 000000000000..ce69e45973fd
--- /dev/null
+++ b/drivers/watchdog/ds1374_wdt.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RTC client/driver for the Maxim/Dallas DS1374 Real-Time Clock over I2C
+ *
+ * Based on code by Randy Vinson <rvinson@mvista.com>,
+ * which was based on the m41t00.c by Mark Greer <mgreer@mvista.com>.
+ *
+ * Copyright (C) 2020 Johnson Chen <johnsonch.chen@moxa.com>
+ * Copyright (C) 2014 Rose Technology
+ * Copyright (C) 2006-2007 Freescale Semiconductor
+ * Copyright (C) 2005 MontaVista Software, Inc.
+ */
+
+/*
+ * It would be more efficient to use i2c msgs/i2c_transfer directly but, as
+ * recommened in .../Documentation/i2c/writing-clients section
+ * "Sending and receiving", using SMBus level communication is preferred.
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/ioctl.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+#include <linux/workqueue.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/uaccess.h>
+
+#define DEVNAME "ds1374_wdt"
+
+#define DS1374_REG_WDALM0 0x04 /* Watchdog/Alarm */
+#define DS1374_REG_WDALM1 0x05
+#define DS1374_REG_WDALM2 0x06
+#define DS1374_REG_CR 0x07 /* Control */
+#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
+#define DS1374_REG_CR_WDSTR 0x08 /* 1=INT, 0=RST */
+#define DS1374_REG_CR_WDALM 0x20 /* 1=Watchdog, 0=Alarm */
+#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
+
+/* Default margin */
+#define WD_TIMO 131762
+#define TIMER_MARGIN_MIN 4096 /* 1s */
+#define TIMER_MARGIN_MAX (60*60*24*4096) /* one day */
+
+static int wdt_margin = WD_TIMO;
+module_param(wdt_margin, int, 0);
+MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 32s)");
+
+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) ")");
+
+struct ds1374_wdt {
+ struct i2c_client *client;
+ struct watchdog_device *wdt;
+ struct work_struct work;
+
+ /* The mutex protects alarm operations, and prevents a race
+ * between the enable_irq() in the workqueue and the free_irq()
+ * in the remove function.
+ */
+ struct mutex mutex;
+};
+
+static const struct watchdog_info ds1374_wdt_info = {
+ .identity = DEVNAME,
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+ WDIOF_MAGICCLOSE,
+};
+
+static struct watchdog_device ds1374_wdd;
+
+static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
+ unsigned int timeout)
+{
+ int ret = -ENOIOCTLCMD;
+ u8 buf[4];
+ int cr, i;
+ struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
+
+ ret = cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
+ if (ret < 0)
+ goto out;
+
+ /* Disable any existing watchdog/alarm before setting the new one */
+ cr &= ~DS1374_REG_CR_WACE;
+
+ ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
+ if (ret < 0)
+ goto out;
+
+ /* Set new watchdog time */
+ for (i = 0; i < 3; i++) {
+ buf[i] = timeout & 0xff;
+ timeout >>= 8;
+ }
+
+ ret = i2c_smbus_write_i2c_block_data(drv_data->client,
+ DS1374_REG_WDALM0, 3, buf);
+ if (ret) {
+ pr_info("couldn't set new watchdog time\n");
+ goto out;
+ }
+
+ /* Enable watchdog timer */
+ cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
+ cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
+ cr &= ~DS1374_REG_CR_AIE;
+
+ ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
+ if (ret < 0)
+ goto out;
+
+ return 0;
+out:
+ return ret;
+}
+
+
+/*
+ * Read WD/Alarm counter to reload the watchdog timer. (ie, pat the watchdog)
+ */
+static int ds1374_wdt_ping(struct watchdog_device *wdt)
+{
+ struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
+ int ret;
+ u8 buf[4];
+
+ ret = i2c_smbus_read_i2c_block_data(drv_data->client,
+ DS1374_REG_WDALM0, 3, buf);
+
+ if (ret < 0 || ret < 3)
+ pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret);
+
+ return ret;
+}
+
+static int ds1374_wdt_start(struct watchdog_device *wdt)
+{
+ int ret;
+
+ ret = ds1374_wdt_settimeout(wdt, wdt_margin);
+ if (ret)
+ return ret;
+
+ ds1374_wdt_ping(wdt);
+
+ return 0;
+}
+
+static int ds1374_wdt_stop(struct watchdog_device *wdt)
+{
+ struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
+ int cr;
+
+ if (nowayout)
+ return 0;
+
+ cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
+ /* Disable watchdog timer */
+ cr &= ~DS1374_REG_CR_WACE;
+
+ return i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
+}
+
+/*
+ * Handle commands from user-space.
+ */
+static long ds1374_wdt_ioctl(struct watchdog_device *wdt, unsigned int cmd,
+ unsigned long arg)
+{
+ int new_margin, options;
+
+ switch (cmd) {
+ case WDIOC_GETSUPPORT:
+ return copy_to_user((struct watchdog_info __user *)arg,
+ &ds1374_wdt_info, sizeof(ds1374_wdt_info)) ? -EFAULT : 0;
+
+ case WDIOC_GETSTATUS:
+ case WDIOC_GETBOOTSTATUS:
+ return put_user(0, (int __user *)arg);
+ case WDIOC_KEEPALIVE:
+ ds1374_wdt_ping(wdt);
+ return 0;
+ case WDIOC_SETTIMEOUT:
+ if (get_user(new_margin, (int __user *)arg))
+ return -EFAULT;
+
+ /* the hardware's tick rate is 4096 Hz, so
+ * the counter value needs to be scaled accordingly
+ */
+ new_margin <<= 12;
+ if (new_margin < 1 || new_margin > 16777216)
+ return -EINVAL;
+
+ wdt_margin = new_margin;
+ ds1374_wdt_settimeout(wdt, new_margin);
+ ds1374_wdt_ping(wdt);
+ fallthrough;
+ case WDIOC_GETTIMEOUT:
+ /* when returning ... inverse is true */
+ return put_user((wdt_margin >> 12), (int __user *)arg);
+ case WDIOC_SETOPTIONS:
+ if (copy_from_user(&options, (int __user *)arg, sizeof(int)))
+ return -EFAULT;
+
+ if (options & WDIOS_DISABLECARD) {
+ pr_info("disable watchdog\n");
+ ds1374_wdt_stop(wdt);
+ return 0;
+ }
+
+ if (options & WDIOS_ENABLECARD) {
+ pr_info("enable watchdog\n");
+ ds1374_wdt_settimeout(wdt, wdt_margin);
+ ds1374_wdt_ping(wdt);
+ return 0;
+ }
+ return -EINVAL;
+ }
+ return -ENOTTY;
+}
+
+static int ds1374_wdt_notify_sys(struct notifier_block *this,
+ unsigned long code, void *unused)
+{
+ if (code == SYS_DOWN || code == SYS_HALT)
+ /* Disable Watchdog */
+ ds1374_wdt_stop(&ds1374_wdd);
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block ds1374_wdt_notifier = {
+ .notifier_call = ds1374_wdt_notify_sys,
+};
+
+static int ds1374_wdt_probe(struct platform_device *pdev)
+{
+ struct ds1374_wdt *drv_data;
+ struct i2c_client *client = to_i2c_client(pdev->dev.parent);
+ int ret;
+
+ drv_data = devm_kzalloc(&pdev->dev, sizeof(struct ds1374_wdt),
+ GFP_KERNEL);
+ if (!drv_data)
+ return -ENOMEM;
+
+ drv_data->wdt = &ds1374_wdd;
+ drv_data->client = client;
+ mutex_init(&drv_data->mutex);
+
+ watchdog_init_timeout(drv_data->wdt, wdt_margin, &pdev->dev);
+ watchdog_set_nowayout(drv_data->wdt, nowayout);
+
+ watchdog_set_drvdata(drv_data->wdt, drv_data);
+ platform_set_drvdata(pdev, drv_data);
+
+ ret = watchdog_register_device(drv_data->wdt);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register Watchdog device\n");
+ return ret;
+ }
+
+ ret = register_reboot_notifier(&ds1374_wdt_notifier);
+ if (ret) {
+ watchdog_unregister_device(drv_data->wdt);
+ return ret;
+ }
+
+ ds1374_wdt_settimeout(drv_data->wdt, wdt_margin);
+ dev_info(&pdev->dev, "Dallas/Maxim DS1374 watchdog device enabled\n");
+
+ return 0;
+}
+
+static int ds1374_wdt_remove(struct platform_device *pdev)
+{
+ struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
+
+ dev_warn(&pdev->dev, "Unregister DS1374 watchdog device\n");
+ watchdog_unregister_device(drv_data->wdt);
+ unregister_reboot_notifier(&ds1374_wdt_notifier);
+
+ return 0;
+}
+
+static void ds1374_wdt_shutdown(struct platform_device *pdev)
+{
+ struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
+
+ mutex_lock(&drv_data->mutex);
+ ds1374_wdt_stop(drv_data->wdt);
+ mutex_unlock(&drv_data->mutex);
+}
+
+static const struct watchdog_ops ds1374_wdt_fops = {
+ .owner = THIS_MODULE,
+ .start = ds1374_wdt_start,
+ .stop = ds1374_wdt_stop,
+ .ping = ds1374_wdt_ping,
+ .set_timeout = ds1374_wdt_settimeout,
+ .ioctl = ds1374_wdt_ioctl,
+};
+
+static struct watchdog_device ds1374_wdd = {
+ .info = &ds1374_wdt_info,
+ .ops = &ds1374_wdt_fops,
+ .min_timeout = TIMER_MARGIN_MIN,
+ .max_timeout = TIMER_MARGIN_MAX,
+};
+
+static struct platform_driver ds1374_wdt = {
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = DEVNAME,
+ },
+ .probe = ds1374_wdt_probe,
+ .remove = ds1374_wdt_remove,
+ .shutdown = ds1374_wdt_shutdown,
+};
+
+module_platform_driver(ds1374_wdt);
+
+MODULE_AUTHOR("Johnson Chen <johnsonch.chen@moxa.com>");
+MODULE_DESCRIPTION("Dallas/Maxim DS1374 Watchdog Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("Platform:ds1374_wdt");
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver
2020-06-22 10:03 [PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver Johnson CH Chen (陳昭勳)
@ 2020-06-22 14:26 ` Guenter Roeck
2020-06-23 6:28 ` Johnson CH Chen (陳昭勳)
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2020-06-22 14:26 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳), linux-kernel
Cc: linux-rtc, linux-watchdog, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Lee Jones
On 6/22/20 3:03 AM, Johnson CH Chen (陳昭勳) wrote:
> Here provide Watchdog function from rtc-ds1374.c which is in RTC subsystem
> originally. Besides, add nowayout and implement Watchdog margin time when
> DS1374 Watchdog device is found.
>
> Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> ---
> drivers/watchdog/Kconfig | 11 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/ds1374_wdt.c | 330 ++++++++++++++++++++++++++++++++++
> 3 files changed, 342 insertions(+)
> create mode 100644 drivers/watchdog/ds1374_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index b739c476955b..b4ff4b3c18da 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -316,6 +316,17 @@ config ZIIRAVE_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called ziirave_wdt.
>
> +config DS1374_WATCHDOG
> + tristate "Dallas/Maxim DS1374 Watchdog timer"
> + depends on MFD_DS1374
> + select WATCHDOG_CORE
> + help
> + If you say Y here you will get support for the watchdog timer
> + in the Dallas/Maxim Semiconductor DS1374 real-time clock chips.
> +
> + This driver can also be built as a module. If so the module
> + will be called ds1374_wdt.
> +
> config RAVE_SP_WATCHDOG
> tristate "RAVE SP Watchdog timer"
> depends on RAVE_SP_CORE
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 6de2e4ceef19..1c468f5d9e5f 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -224,3 +224,4 @@ obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
> obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
> obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
> obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
> +obj-$(CONFIG_DS1374_WATCHDOG) += ds1374_wdt.o
> diff --git a/drivers/watchdog/ds1374_wdt.c b/drivers/watchdog/ds1374_wdt.c
> new file mode 100644
> index 000000000000..ce69e45973fd
> --- /dev/null
> +++ b/drivers/watchdog/ds1374_wdt.c
> @@ -0,0 +1,330 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RTC client/driver for the Maxim/Dallas DS1374 Real-Time Clock over I2C
> + *
> + * Based on code by Randy Vinson <rvinson@mvista.com>,
> + * which was based on the m41t00.c by Mark Greer <mgreer@mvista.com>.
> + *
> + * Copyright (C) 2020 Johnson Chen <johnsonch.chen@moxa.com>
> + * Copyright (C) 2014 Rose Technology
> + * Copyright (C) 2006-2007 Freescale Semiconductor
> + * Copyright (C) 2005 MontaVista Software, Inc.
> + */
> +
> +/*
> + * It would be more efficient to use i2c msgs/i2c_transfer directly but, as
> + * recommened in .../Documentation/i2c/writing-clients section
> + * "Sending and receiving", using SMBus level communication is preferred.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/ioctl.h>
> +#include <linux/reboot.h>
> +#include <linux/watchdog.h>
> +#include <linux/workqueue.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/uaccess.h>
Alphabetic order please.
> +
> +#define DEVNAME "ds1374_wdt"
> +
> +#define DS1374_REG_WDALM0 0x04 /* Watchdog/Alarm */
> +#define DS1374_REG_WDALM1 0x05
> +#define DS1374_REG_WDALM2 0x06
> +#define DS1374_REG_CR 0x07 /* Control */
> +#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
> +#define DS1374_REG_CR_WDSTR 0x08 /* 1=INT, 0=RST */
> +#define DS1374_REG_CR_WDALM 0x20 /* 1=Watchdog, 0=Alarm */
> +#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
> +
> +/* Default margin */
> +#define WD_TIMO 131762
> +#define TIMER_MARGIN_MIN 4096 /* 1s */
> +#define TIMER_MARGIN_MAX (60*60*24*4096) /* one day */
> +
> +static int wdt_margin = WD_TIMO;
Sjould not be pre-initialized. Also, 131762 isn't 32 seconds,
it is 131,762 seconds.
> +module_param(wdt_margin, int, 0);
> +MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 32s)");
> +
As a new driver, it would be better to just use the customary "timeout".
> +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) ")");
> +
> +struct ds1374_wdt {
> + struct i2c_client *client;
> + struct watchdog_device *wdt;
> + struct work_struct work;
Not used.
> +
> + /* The mutex protects alarm operations, and prevents a race
> + * between the enable_irq() in the workqueue and the free_irq()
> + * in the remove function.
> + */
There is no workqueue here, and the remove function is not needed.
> + struct mutex mutex;
> +};
> +
> +static const struct watchdog_info ds1374_wdt_info = {
> + .identity = DEVNAME,
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> + WDIOF_MAGICCLOSE,
> +};
> +
> +static struct watchdog_device ds1374_wdd;
> +
Move declaration please.
> +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
> + unsigned int timeout)
> +{
How is this synchronized against accesses by the RTC driver ?
> + int ret = -ENOIOCTLCMD;
Not an appropriate error code here.
> + u8 buf[4];
> + int cr, i;
> + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> +
> + ret = cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
> + if (ret < 0)
> + goto out;
"goto out;" is unnecessary. Just return the error. Same everywhere else below.
> +
> + /* Disable any existing watchdog/alarm before setting the new one */
> + cr &= ~DS1374_REG_CR_WACE;
> +
> + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
> + if (ret < 0)
> + goto out;
> +
> + /* Set new watchdog time */
> + for (i = 0; i < 3; i++) {
> + buf[i] = timeout & 0xff;
> + timeout >>= 8;
> + }
The value passed to this function from the watchdog core is the
timeout in seconds. I don't see a conversion to internal values
here.
> +
> + ret = i2c_smbus_write_i2c_block_data(drv_data->client,
> + DS1374_REG_WDALM0, 3, buf);
> + if (ret) {
> + pr_info("couldn't set new watchdog time\n");
> + goto out;
> + }
> +
> + /* Enable watchdog timer */
> + cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
> + cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
> + cr &= ~DS1374_REG_CR_AIE;
> +
> + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
> + if (ret < 0)
> + goto out;
> +
> + return 0;
Pointless. Just return ret.
Also, this function needs to store the new timeout in struct watchdog_device.
> +out:
> + return ret;
> +}
> +
> +
> +/*
> + * Read WD/Alarm counter to reload the watchdog timer. (ie, pat the watchdog)
> + */
> +static int ds1374_wdt_ping(struct watchdog_device *wdt)
> +{
> + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> + int ret;
> + u8 buf[4];
> +
> + ret = i2c_smbus_read_i2c_block_data(drv_data->client,
> + DS1374_REG_WDALM0, 3, buf);
> +
> + if (ret < 0 || ret < 3)
> + pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret);
> +
This is not an info message, this is an error. Besides, it is just noise.
Just return the error and drop the message.
> + return ret;
> +}
> +
> +static int ds1374_wdt_start(struct watchdog_device *wdt)
> +{
> + int ret;
> +
> + ret = ds1374_wdt_settimeout(wdt, wdt_margin);
This is wrong. The timeout may have been updated by userspace.
It is inappropriate to change it back to the default here.
> + if (ret)
> + return ret;
> +
> + ds1374_wdt_ping(wdt);
Please do not ignore errors.
> +
> + return 0;
> +}
> +
> +static int ds1374_wdt_stop(struct watchdog_device *wdt)
> +{
> + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> + int cr;
> +
> + if (nowayout)
> + return 0;
Not needed.
> +
> + cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
> + /* Disable watchdog timer */
> + cr &= ~DS1374_REG_CR_WACE;
> +
> + return i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
> +}
> +
> +/*
> + * Handle commands from user-space.
> + */
> +static long ds1374_wdt_ioctl(struct watchdog_device *wdt, unsigned int cmd,
> + unsigned long arg)
The whole point of using the watchdog subsystem is to not need a local
ioctl function - and most definitely not one that duplicates watchdog
core functionality.
> +{
> + int new_margin, options;
> +
> + switch (cmd) {
> + case WDIOC_GETSUPPORT:
> + return copy_to_user((struct watchdog_info __user *)arg,
> + &ds1374_wdt_info, sizeof(ds1374_wdt_info)) ? -EFAULT : 0;
> +
> + case WDIOC_GETSTATUS:
> + case WDIOC_GETBOOTSTATUS:
> + return put_user(0, (int __user *)arg);
> + case WDIOC_KEEPALIVE:
> + ds1374_wdt_ping(wdt);
> + return 0;
> + case WDIOC_SETTIMEOUT:
> + if (get_user(new_margin, (int __user *)arg))
> + return -EFAULT;
> +
> + /* the hardware's tick rate is 4096 Hz, so
> + * the counter value needs to be scaled accordingly
> + */
> + new_margin <<= 12;
> + if (new_margin < 1 || new_margin > 16777216)
> + return -EINVAL;
> +
> + wdt_margin = new_margin;
> + ds1374_wdt_settimeout(wdt, new_margin);
Is the idea here to bypass the watchdog subsystem's notion of
keeping the timeout in seconds ? If so, that would be wrong
and unacceptable.
> + ds1374_wdt_ping(wdt);
> + fallthrough;
> + case WDIOC_GETTIMEOUT:
> + /* when returning ... inverse is true */
> + return put_user((wdt_margin >> 12), (int __user *)arg);
> + case WDIOC_SETOPTIONS:
> + if (copy_from_user(&options, (int __user *)arg, sizeof(int)))
> + return -EFAULT;
> +
> + if (options & WDIOS_DISABLECARD) {
> + pr_info("disable watchdog\n");
> + ds1374_wdt_stop(wdt);
> + return 0;
> + }
> +
> + if (options & WDIOS_ENABLECARD) {
> + pr_info("enable watchdog\n");
> + ds1374_wdt_settimeout(wdt, wdt_margin);
> + ds1374_wdt_ping(wdt);
> + return 0;
> + }
> + return -EINVAL;
> + }
> + return -ENOTTY;
> +}
> +
> +static int ds1374_wdt_notify_sys(struct notifier_block *this,
> + unsigned long code, void *unused)
> +{
> + if (code == SYS_DOWN || code == SYS_HALT)
> + /* Disable Watchdog */
> + ds1374_wdt_stop(&ds1374_wdd);
> + return NOTIFY_DONE;
> +}
> +
Not needed - see below.
> +static struct notifier_block ds1374_wdt_notifier = {
> + .notifier_call = ds1374_wdt_notify_sys,
> +};
> +
> +static int ds1374_wdt_probe(struct platform_device *pdev)
> +{
> + struct ds1374_wdt *drv_data;
> + struct i2c_client *client = to_i2c_client(pdev->dev.parent);
> + int ret;
> +
> + drv_data = devm_kzalloc(&pdev->dev, sizeof(struct ds1374_wdt),
> + GFP_KERNEL);
> + if (!drv_data)
> + return -ENOMEM;
> +
> + drv_data->wdt = &ds1374_wdd;
> + drv_data->client = client;
> + mutex_init(&drv_data->mutex);
> +
> + watchdog_init_timeout(drv_data->wdt, wdt_margin, &pdev->dev);
> + watchdog_set_nowayout(drv_data->wdt, nowayout);
> +
> + watchdog_set_drvdata(drv_data->wdt, drv_data);
> + platform_set_drvdata(pdev, drv_data);
> +
> + ret = watchdog_register_device(drv_data->wdt);
Use devm_watchdog_register_device()
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register Watchdog device\n");
> + return ret;
> + }
> +
> + ret = register_reboot_notifier(&ds1374_wdt_notifier);
Call watchdog_stop_on_reboot() before calling watchdog_register_device()
instead.
> + if (ret) {
> + watchdog_unregister_device(drv_data->wdt);
> + return ret;
> + }
> +
> + ds1374_wdt_settimeout(drv_data->wdt, wdt_margin);
Unnecessary here; called from start function.
> + dev_info(&pdev->dev, "Dallas/Maxim DS1374 watchdog device enabled\n");
> +
> + return 0;
> +}
> +
> +static int ds1374_wdt_remove(struct platform_device *pdev)
> +{
> + struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
> +
> + dev_warn(&pdev->dev, "Unregister DS1374 watchdog device\n");
> + watchdog_unregister_device(drv_data->wdt);
> + unregister_reboot_notifier(&ds1374_wdt_notifier);
> +
Call watchdog_stop_on_unregister() before calling watchdog_register_device().
> + return 0;
> +}
> +
> +static void ds1374_wdt_shutdown(struct platform_device *pdev)
> +{
> + struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
> +
> + mutex_lock(&drv_data->mutex);
> + ds1374_wdt_stop(drv_data->wdt);
> + mutex_unlock(&drv_data->mutex);
Unnecessary and pointless.
> +}
> +
> +static const struct watchdog_ops ds1374_wdt_fops = {
> + .owner = THIS_MODULE,
> + .start = ds1374_wdt_start,
> + .stop = ds1374_wdt_stop,
> + .ping = ds1374_wdt_ping,
> + .set_timeout = ds1374_wdt_settimeout,
> + .ioctl = ds1374_wdt_ioctl,
> +};
> +
> +static struct watchdog_device ds1374_wdd = {
> + .info = &ds1374_wdt_info,
> + .ops = &ds1374_wdt_fops,
> + .min_timeout = TIMER_MARGIN_MIN,
> + .max_timeout = TIMER_MARGIN_MAX,
.timeout should be set here.
Also, there can (at least in theory) be more than one ds1374
in the system. The code does not support this case. ds1374_wdd
should be moved into struct ds1374_wdt.
> +};
> +
> +static struct platform_driver ds1374_wdt = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = DEVNAME,
> + },
> + .probe = ds1374_wdt_probe,
> + .remove = ds1374_wdt_remove,
> + .shutdown = ds1374_wdt_shutdown,
> +};
> +
> +module_platform_driver(ds1374_wdt);
> +
> +MODULE_AUTHOR("Johnson Chen <johnsonch.chen@moxa.com>");
> +MODULE_DESCRIPTION("Dallas/Maxim DS1374 Watchdog Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("Platform:ds1374_wdt");
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver
2020-06-22 14:26 ` Guenter Roeck
@ 2020-06-23 6:28 ` Johnson CH Chen (陳昭勳)
2020-06-23 13:20 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-06-23 6:28 UTC (permalink / raw)
To: Guenter Roeck, linux-kernel
Cc: linux-rtc, linux-watchdog, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Lee Jones
Hi,
Thanks for your good detailed suggestions!
> > + * It would be more efficient to use i2c msgs/i2c_transfer directly
> > +but, as
> > + * recommened in .../Documentation/i2c/writing-clients section
> > + * "Sending and receiving", using SMBus level communication is preferred.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/ioctl.h>
> > +#include <linux/reboot.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/uaccess.h>
>
> Alphabetic order please.
>
> > +
> > +#define DEVNAME "ds1374_wdt"
> > +
> > +#define DS1374_REG_WDALM0 0x04 /* Watchdog/Alarm */
> > +#define DS1374_REG_WDALM1 0x05
> > +#define DS1374_REG_WDALM2 0x06
> > +#define DS1374_REG_CR 0x07 /* Control */
> > +#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
> > +#define DS1374_REG_CR_WDSTR 0x08 /* 1=INT, 0=RST */
> > +#define DS1374_REG_CR_WDALM 0x20 /* 1=Watchdog, 0=Alarm */
> > +#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
> > +
> > +/* Default margin */
> > +#define WD_TIMO 131762
> > +#define TIMER_MARGIN_MIN 4096 /* 1s */
> > +#define TIMER_MARGIN_MAX (60*60*24*4096) /* one day */
> > +
> > +static int wdt_margin = WD_TIMO;
>
> Sjould not be pre-initialized. Also, 131762 isn't 32 seconds, it is 131,762
> seconds.
>
131762 is 32 seconds actually because watchdog counter increases per
1/4096 seconds for DS1374. If DS1374_REG_CR_WDALM is set to 0 (alarm),
alarm counter will increase per 1 second.
> > +module_param(wdt_margin, int, 0);
> > +MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default
> > +32s)");
> > +
>
> As a new driver, it would be better to just use the customary "timeout".
>
> > +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) ")");
> > +
> > +struct ds1374_wdt {
> > + struct i2c_client *client;
> > + struct watchdog_device *wdt;
> > + struct work_struct work;
>
> Not used.
>
> > +
> > + /* The mutex protects alarm operations, and prevents a race
> > + * between the enable_irq() in the workqueue and the free_irq()
> > + * in the remove function.
> > + */
>
> There is no workqueue here, and the remove function is not needed.
>
> > + struct mutex mutex;
> > +};
> > +
> > +static const struct watchdog_info ds1374_wdt_info = {
> > + .identity = DEVNAME,
> > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> > + WDIOF_MAGICCLOSE,
> > +};
> > +
> > +static struct watchdog_device ds1374_wdd;
> > +
> Move declaration please.
>
> > +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
> > + unsigned int timeout)
> > +{
>
> How is this synchronized against accesses by the RTC driver ?
>
By original design in rtc-ds1374.c, it seems no synchronized problem between
rtc and watchdog, but I think we can add mutex lock to deal with it.
> > + int ret = -ENOIOCTLCMD;
>
> Not an appropriate error code here.
>
> > + u8 buf[4];
> > + int cr, i;
> > + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> > +
> > + ret = cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
> > + if (ret < 0)
> > + goto out;
>
> "goto out;" is unnecessary. Just return the error. Same everywhere else below.
>
> > +
> > + /* Disable any existing watchdog/alarm before setting the new one */
> > + cr &= ~DS1374_REG_CR_WACE;
> > +
> > + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
> > + if (ret < 0)
> > + goto out;
> > +
> > + /* Set new watchdog time */
> > + for (i = 0; i < 3; i++) {
> > + buf[i] = timeout & 0xff;
> > + timeout >>= 8;
> > + }
>
> The value passed to this function from the watchdog core is the timeout in
> seconds. I don't see a conversion to internal values here.
>
For original design in rtc-ds1374.c, ds1374_wdt_settimeout () will call
ds1374_write_rtc() to write hardware register. To separate watchdog and rtc
functions, expand code from ds1374_write_rtc() here, and final buf[] values
will be written into hardware register for DS1374.
> > +
> > + ret = i2c_smbus_write_i2c_block_data(drv_data->client,
> > + DS1374_REG_WDALM0, 3, buf);
> > + if (ret) {
> > + pr_info("couldn't set new watchdog time\n");
> > + goto out;
> > + }
>
> > +
> > + /* Enable watchdog timer */
> > + cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
> > + cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
> > + cr &= ~DS1374_REG_CR_AIE;
> > +
> > + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
> > + if (ret < 0)
> > + goto out;
> > +
> > + return 0;
>
> Pointless. Just return ret.
>
> Also, this function needs to store the new timeout in struct watchdog_device.
>
> > +out:
> > + return ret;
> > +}
> > +
> > +
> > +/*
> > + * Read WD/Alarm counter to reload the watchdog timer. (ie, pat the
> > +watchdog) */ static int ds1374_wdt_ping(struct watchdog_device *wdt)
> > +{
> > + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> > + int ret;
> > + u8 buf[4];
> > +
> > + ret = i2c_smbus_read_i2c_block_data(drv_data->client,
> > + DS1374_REG_WDALM0, 3, buf);
> > +
> > + if (ret < 0 || ret < 3)
> > + pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret);
> > +
>
> This is not an info message, this is an error. Besides, it is just noise.
> Just return the error and drop the message.
>
> > + return ret;
> > +}
> > +
> > +static int ds1374_wdt_start(struct watchdog_device *wdt) {
> > + int ret;
> > +
> > + ret = ds1374_wdt_settimeout(wdt, wdt_margin);
>
> This is wrong. The timeout may have been updated by userspace.
> It is inappropriate to change it back to the default here.
>
Thanks, I will keep in mind.
> > + if (ret)
> > + return ret;
> > +
> > + ds1374_wdt_ping(wdt);
>
> Please do not ignore errors.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int ds1374_wdt_stop(struct watchdog_device *wdt) {
> > + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> > + int cr;
> > +
> > + if (nowayout)
> > + return 0;
>
> Not needed.
>
Thanks!, it has been implemented in watchdog_stop().
> > +
> > + cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
> > + /* Disable watchdog timer */
> > + cr &= ~DS1374_REG_CR_WACE;
> > +
> > + return i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR,
> > +cr); }
> > +
> > +/*
> > + * Handle commands from user-space.
> > + */
> > +static long ds1374_wdt_ioctl(struct watchdog_device *wdt, unsigned int
> cmd,
> > + unsigned long arg)
>
> The whole point of using the watchdog subsystem is to not need a local ioctl
> function - and most definitely not one that duplicates watchdog core
> functionality.
>
> > +{
> > + int new_margin, options;
> > +
> > + switch (cmd) {
> > + case WDIOC_GETSUPPORT:
> > + return copy_to_user((struct watchdog_info __user *)arg,
> > + &ds1374_wdt_info, sizeof(ds1374_wdt_info)) ? -EFAULT : 0;
> > +
> > + case WDIOC_GETSTATUS:
> > + case WDIOC_GETBOOTSTATUS:
> > + return put_user(0, (int __user *)arg);
> > + case WDIOC_KEEPALIVE:
> > + ds1374_wdt_ping(wdt);
> > + return 0;
> > + case WDIOC_SETTIMEOUT:
> > + if (get_user(new_margin, (int __user *)arg))
> > + return -EFAULT;
> > +
> > + /* the hardware's tick rate is 4096 Hz, so
> > + * the counter value needs to be scaled accordingly
> > + */
> > + new_margin <<= 12;
> > + if (new_margin < 1 || new_margin > 16777216)
> > + return -EINVAL;
> > +
> > + wdt_margin = new_margin;
> > + ds1374_wdt_settimeout(wdt, new_margin);
>
> Is the idea here to bypass the watchdog subsystem's notion of keeping the
> timeout in seconds ? If so, that would be wrong and unacceptable.
>
It seems take care about 4096Hz for original design in rtc-ds1374.c. I think we
can just use ioctl() which watchdog core provides and input margin time
appropriately.
> > + ds1374_wdt_ping(wdt);
> > + fallthrough;
> > + case WDIOC_GETTIMEOUT:
> > + /* when returning ... inverse is true */
> > + return put_user((wdt_margin >> 12), (int __user *)arg);
> > + case WDIOC_SETOPTIONS:
> > + if (copy_from_user(&options, (int __user *)arg, sizeof(int)))
> > + return -EFAULT;
> > +
> > + if (options & WDIOS_DISABLECARD) {
> > + pr_info("disable watchdog\n");
> > + ds1374_wdt_stop(wdt);
> > + return 0;
> > + }
> > +
> > + if (options & WDIOS_ENABLECARD) {
> > + pr_info("enable watchdog\n");
> > + ds1374_wdt_settimeout(wdt, wdt_margin);
> > + ds1374_wdt_ping(wdt);
> > + return 0;
> > + }
> > + return -EINVAL;
> > + }
> > + return -ENOTTY;
> > +}
> > +
> > +static int ds1374_wdt_notify_sys(struct notifier_block *this,
> > + unsigned long code, void *unused)
> > +{
> > + if (code == SYS_DOWN || code == SYS_HALT)
> > + /* Disable Watchdog */
> > + ds1374_wdt_stop(&ds1374_wdd);
> > + return NOTIFY_DONE;
> > +}
> > +
> Not needed - see below.
>
> > +static struct notifier_block ds1374_wdt_notifier = {
> > + .notifier_call = ds1374_wdt_notify_sys, };
> > +
> > +static int ds1374_wdt_probe(struct platform_device *pdev) {
> > + struct ds1374_wdt *drv_data;
> > + struct i2c_client *client = to_i2c_client(pdev->dev.parent);
> > + int ret;
> > +
> > + drv_data = devm_kzalloc(&pdev->dev, sizeof(struct ds1374_wdt),
> > + GFP_KERNEL);
> > + if (!drv_data)
> > + return -ENOMEM;
> > +
> > + drv_data->wdt = &ds1374_wdd;
> > + drv_data->client = client;
> > + mutex_init(&drv_data->mutex);
> > +
> > + watchdog_init_timeout(drv_data->wdt, wdt_margin, &pdev->dev);
> > + watchdog_set_nowayout(drv_data->wdt, nowayout);
> > +
> > + watchdog_set_drvdata(drv_data->wdt, drv_data);
> > + platform_set_drvdata(pdev, drv_data);
> > +
> > + ret = watchdog_register_device(drv_data->wdt);
>
> Use devm_watchdog_register_device()
>
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to register Watchdog device\n");
> > + return ret;
> > + }
> > +
> > + ret = register_reboot_notifier(&ds1374_wdt_notifier);
>
> Call watchdog_stop_on_reboot() before calling watchdog_register_device()
> instead.
>
> > + if (ret) {
> > + watchdog_unregister_device(drv_data->wdt);
> > + return ret;
> > + }
> > +
> > + ds1374_wdt_settimeout(drv_data->wdt, wdt_margin);
>
> Unnecessary here; called from start function.
>
> > + dev_info(&pdev->dev, "Dallas/Maxim DS1374 watchdog device
> > +enabled\n");
> > +
> > + return 0;
> > +}
> > +
> > +static int ds1374_wdt_remove(struct platform_device *pdev) {
> > + struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
> > +
> > + dev_warn(&pdev->dev, "Unregister DS1374 watchdog device\n");
> > + watchdog_unregister_device(drv_data->wdt);
> > + unregister_reboot_notifier(&ds1374_wdt_notifier);
> > +
>
> Call watchdog_stop_on_unregister() before calling
> watchdog_register_device().
>
> > + return 0;
> > +}
> > +
> > +static void ds1374_wdt_shutdown(struct platform_device *pdev) {
> > + struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
> > +
> > + mutex_lock(&drv_data->mutex);
> > + ds1374_wdt_stop(drv_data->wdt);
> > + mutex_unlock(&drv_data->mutex);
>
> Unnecessary and pointless.
>
> > +}
> > +
> > +static const struct watchdog_ops ds1374_wdt_fops = {
> > + .owner = THIS_MODULE,
> > + .start = ds1374_wdt_start,
> > + .stop = ds1374_wdt_stop,
> > + .ping = ds1374_wdt_ping,
> > + .set_timeout = ds1374_wdt_settimeout,
> > + .ioctl = ds1374_wdt_ioctl,
> > +};
> > +
> > +static struct watchdog_device ds1374_wdd = {
> > + .info = &ds1374_wdt_info,
> > + .ops = &ds1374_wdt_fops,
> > + .min_timeout = TIMER_MARGIN_MIN,
> > + .max_timeout = TIMER_MARGIN_MAX,
>
> .timeout should be set here.
>
> Also, there can (at least in theory) be more than one ds1374 in the system. The
> code does not support this case. ds1374_wdd should be moved into struct
> ds1374_wdt.
>
Thanks for good suggestion.
> > +};
> > +
> > +static struct platform_driver ds1374_wdt = {
> > + .driver = {
> > + .owner = THIS_MODULE,
> > + .name = DEVNAME,
> > + },
> > + .probe = ds1374_wdt_probe,
> > + .remove = ds1374_wdt_remove,
> > + .shutdown = ds1374_wdt_shutdown,
> > +};
> > +
> > +module_platform_driver(ds1374_wdt);
> > +
> > +MODULE_AUTHOR("Johnson Chen <johnsonch.chen@moxa.com>");
> > +MODULE_DESCRIPTION("Dallas/Maxim DS1374 Watchdog Driver");
> > +MODULE_LICENSE("GPL"); MODULE_ALIAS("Platform:ds1374_wdt");
> >
Best regards,
Johnson
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver
2020-06-23 6:28 ` Johnson CH Chen (陳昭勳)
@ 2020-06-23 13:20 ` Guenter Roeck
2020-06-24 12:09 ` Johnson CH Chen (陳昭勳)
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2020-06-23 13:20 UTC (permalink / raw)
To: Johnson CH Chen (陳昭勳), linux-kernel
Cc: linux-rtc, linux-watchdog, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Lee Jones
On 6/22/20 11:28 PM, Johnson CH Chen (陳昭勳) wrote:
> Hi,
>
> Thanks for your good detailed suggestions!
>
Other feedback suggests to convert the driver to use
the watchdog core in the rtc code. I would suggest to follow
that suggestion.
>>> + * It would be more efficient to use i2c msgs/i2c_transfer directly
>>> +but, as
>>> + * recommened in .../Documentation/i2c/writing-clients section
>>> + * "Sending and receiving", using SMBus level communication is preferred.
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/device.h>
>>> +#include <linux/module.h>
>>> +#include <linux/ioctl.h>
>>> +#include <linux/reboot.h>
>>> +#include <linux/watchdog.h>
>>> +#include <linux/workqueue.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/uaccess.h>
>>
>> Alphabetic order please.
>>
>>> +
>>> +#define DEVNAME "ds1374_wdt"
>>> +
>>> +#define DS1374_REG_WDALM0 0x04 /* Watchdog/Alarm */
>>> +#define DS1374_REG_WDALM1 0x05
>>> +#define DS1374_REG_WDALM2 0x06
>>> +#define DS1374_REG_CR 0x07 /* Control */
>>> +#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
>>> +#define DS1374_REG_CR_WDSTR 0x08 /* 1=INT, 0=RST */
>>> +#define DS1374_REG_CR_WDALM 0x20 /* 1=Watchdog, 0=Alarm */
>>> +#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
>>> +
>>> +/* Default margin */
>>> +#define WD_TIMO 131762
>>> +#define TIMER_MARGIN_MIN 4096 /* 1s */
>>> +#define TIMER_MARGIN_MAX (60*60*24*4096) /* one day */
>>> +
>>> +static int wdt_margin = WD_TIMO;
>>
>> Sjould not be pre-initialized. Also, 131762 isn't 32 seconds, it is 131,762
>> seconds.
>>
>
> 131762 is 32 seconds actually because watchdog counter increases per
> 1/4096 seconds for DS1374. If DS1374_REG_CR_WDALM is set to 0 (alarm),
> alarm counter will increase per 1 second.
>
The watchdog core keeps timeouts in seconds. For the watchdog core,
131762 is 131,762 seconds. Your code assumes that the watchdog core
does not care about / need to scale, which is wrong. Besides,
MODULE_PARM_DESC below clearly states "Watchdog timeout _in seconds_"
(emphasis added).
>>> +module_param(wdt_margin, int, 0);
>>> +MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default
>>> +32s)");
>>> +
>>
>> As a new driver, it would be better to just use the customary "timeout".
>>
>>> +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) ")");
>>> +
>>> +struct ds1374_wdt {
>>> + struct i2c_client *client;
>>> + struct watchdog_device *wdt;
>>> + struct work_struct work;
>>
>> Not used.
>>
>>> +
>>> + /* The mutex protects alarm operations, and prevents a race
>>> + * between the enable_irq() in the workqueue and the free_irq()
>>> + * in the remove function.
>>> + */
>>
>> There is no workqueue here, and the remove function is not needed.
>>
>>> + struct mutex mutex;
>>> +};
>>> +
>>> +static const struct watchdog_info ds1374_wdt_info = {
>>> + .identity = DEVNAME,
>>> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>>> + WDIOF_MAGICCLOSE,
>>> +};
>>> +
>>> +static struct watchdog_device ds1374_wdd;
>>> +
>> Move declaration please.
>>
>>> +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
>>> + unsigned int timeout)
>>> +{
>>
>> How is this synchronized against accesses by the RTC driver ?
>>
> By original design in rtc-ds1374.c, it seems no synchronized problem between
> rtc and watchdog, but I think we can add mutex lock to deal with it.
>
The mutex is used there where needed.
>>> + int ret = -ENOIOCTLCMD;
>>
>> Not an appropriate error code here.
>>
>>> + u8 buf[4];
>>> + int cr, i;
>>> + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
>>> +
>>> + ret = cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
>>> + if (ret < 0)
>>> + goto out;
>>
>> "goto out;" is unnecessary. Just return the error. Same everywhere else below.
>>
>>> +
>>> + /* Disable any existing watchdog/alarm before setting the new one */
>>> + cr &= ~DS1374_REG_CR_WACE;
>>> +
>>> + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
>>> + if (ret < 0)
>>> + goto out;
>>> +
>>> + /* Set new watchdog time */
>>> + for (i = 0; i < 3; i++) {
>>> + buf[i] = timeout & 0xff;
>>> + timeout >>= 8;
>>> + }
>>
>> The value passed to this function from the watchdog core is the timeout in
>> seconds. I don't see a conversion to internal values here.
>>
>
> For original design in rtc-ds1374.c, ds1374_wdt_settimeout () will call
> ds1374_write_rtc() to write hardware register. To separate watchdog and rtc
> functions, expand code from ds1374_write_rtc() here, and final buf[] values
> will be written into hardware register for DS1374.
>
ds1374_wdt_settimeout() is an API function, It gets timeouts from the watchdog
core in seconds. Those timeouts have to be converted to chip internal values
in this function.
>>> +
>>> + ret = i2c_smbus_write_i2c_block_data(drv_data->client,
>>> + DS1374_REG_WDALM0, 3, buf);
>>> + if (ret) {
>>> + pr_info("couldn't set new watchdog time\n");
>>> + goto out;
>>> + }
>>
>>> +
>>> + /* Enable watchdog timer */
>>> + cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
>>> + cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
>>> + cr &= ~DS1374_REG_CR_AIE;
>>> +
>>> + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR, cr);
>>> + if (ret < 0)
>>> + goto out;
>>> +
>>> + return 0;
>>
>> Pointless. Just return ret.
>>
>> Also, this function needs to store the new timeout in struct watchdog_device.
>>
>>> +out:
>>> + return ret;
>>> +}
>>> +
>>> +
>>> +/*
>>> + * Read WD/Alarm counter to reload the watchdog timer. (ie, pat the
>>> +watchdog) */ static int ds1374_wdt_ping(struct watchdog_device *wdt)
>>> +{
>>> + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
>>> + int ret;
>>> + u8 buf[4];
>>> +
>>> + ret = i2c_smbus_read_i2c_block_data(drv_data->client,
>>> + DS1374_REG_WDALM0, 3, buf);
>>> +
>>> + if (ret < 0 || ret < 3)
>>> + pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret);
>>> +
>>
>> This is not an info message, this is an error. Besides, it is just noise.
>> Just return the error and drop the message.
>>
>>> + return ret;
>>> +}
>>> +
>>> +static int ds1374_wdt_start(struct watchdog_device *wdt) {
>>> + int ret;
>>> +
>>> + ret = ds1374_wdt_settimeout(wdt, wdt_margin);
>>
>> This is wrong. The timeout may have been updated by userspace.
>> It is inappropriate to change it back to the default here.
>>
> Thanks, I will keep in mind.
>
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ds1374_wdt_ping(wdt);
>>
>> Please do not ignore errors.
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ds1374_wdt_stop(struct watchdog_device *wdt) {
>>> + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
>>> + int cr;
>>> +
>>> + if (nowayout)
>>> + return 0;
>>
>> Not needed.
>>
> Thanks!, it has been implemented in watchdog_stop().
>
>>> +
>>> + cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
>>> + /* Disable watchdog timer */
>>> + cr &= ~DS1374_REG_CR_WACE;
>>> +
>>> + return i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR,
>>> +cr); }
>>> +
>>> +/*
>>> + * Handle commands from user-space.
>>> + */
>>> +static long ds1374_wdt_ioctl(struct watchdog_device *wdt, unsigned int
>> cmd,
>>> + unsigned long arg)
>>
>> The whole point of using the watchdog subsystem is to not need a local ioctl
>> function - and most definitely not one that duplicates watchdog core
>> functionality.
>>
>>> +{
>>> + int new_margin, options;
>>> +
>>> + switch (cmd) {
>>> + case WDIOC_GETSUPPORT:
>>> + return copy_to_user((struct watchdog_info __user *)arg,
>>> + &ds1374_wdt_info, sizeof(ds1374_wdt_info)) ? -EFAULT : 0;
>>> +
>>> + case WDIOC_GETSTATUS:
>>> + case WDIOC_GETBOOTSTATUS:
>>> + return put_user(0, (int __user *)arg);
>>> + case WDIOC_KEEPALIVE:
>>> + ds1374_wdt_ping(wdt);
>>> + return 0;
>>> + case WDIOC_SETTIMEOUT:
>>> + if (get_user(new_margin, (int __user *)arg))
>>> + return -EFAULT;
>>> +
>>> + /* the hardware's tick rate is 4096 Hz, so
>>> + * the counter value needs to be scaled accordingly
>>> + */
>>> + new_margin <<= 12;
>>> + if (new_margin < 1 || new_margin > 16777216)
>>> + return -EINVAL;
>>> +
>>> + wdt_margin = new_margin;
>>> + ds1374_wdt_settimeout(wdt, new_margin);
>>
>> Is the idea here to bypass the watchdog subsystem's notion of keeping the
>> timeout in seconds ? If so, that would be wrong and unacceptable.
>>
> It seems take care about 4096Hz for original design in rtc-ds1374.c. I think we
> can just use ioctl() which watchdog core provides and input margin time
> appropriately.
>
>
>>> + ds1374_wdt_ping(wdt);
>>> + fallthrough;
>>> + case WDIOC_GETTIMEOUT:
>>> + /* when returning ... inverse is true */
>>> + return put_user((wdt_margin >> 12), (int __user *)arg);
>>> + case WDIOC_SETOPTIONS:
>>> + if (copy_from_user(&options, (int __user *)arg, sizeof(int)))
>>> + return -EFAULT;
>>> +
>>> + if (options & WDIOS_DISABLECARD) {
>>> + pr_info("disable watchdog\n");
>>> + ds1374_wdt_stop(wdt);
>>> + return 0;
>>> + }
>>> +
>>> + if (options & WDIOS_ENABLECARD) {
>>> + pr_info("enable watchdog\n");
>>> + ds1374_wdt_settimeout(wdt, wdt_margin);
>>> + ds1374_wdt_ping(wdt);
>>> + return 0;
>>> + }
>>> + return -EINVAL;
>>> + }
>>> + return -ENOTTY;
>>> +}
>>> +
>>> +static int ds1374_wdt_notify_sys(struct notifier_block *this,
>>> + unsigned long code, void *unused)
>>> +{
>>> + if (code == SYS_DOWN || code == SYS_HALT)
>>> + /* Disable Watchdog */
>>> + ds1374_wdt_stop(&ds1374_wdd);
>>> + return NOTIFY_DONE;
>>> +}
>>> +
>> Not needed - see below.
>>
>>> +static struct notifier_block ds1374_wdt_notifier = {
>>> + .notifier_call = ds1374_wdt_notify_sys, };
>>> +
>>> +static int ds1374_wdt_probe(struct platform_device *pdev) {
>>> + struct ds1374_wdt *drv_data;
>>> + struct i2c_client *client = to_i2c_client(pdev->dev.parent);
>>> + int ret;
>>> +
>>> + drv_data = devm_kzalloc(&pdev->dev, sizeof(struct ds1374_wdt),
>>> + GFP_KERNEL);
>>> + if (!drv_data)
>>> + return -ENOMEM;
>>> +
>>> + drv_data->wdt = &ds1374_wdd;
>>> + drv_data->client = client;
>>> + mutex_init(&drv_data->mutex);
>>> +
>>> + watchdog_init_timeout(drv_data->wdt, wdt_margin, &pdev->dev);
>>> + watchdog_set_nowayout(drv_data->wdt, nowayout);
>>> +
>>> + watchdog_set_drvdata(drv_data->wdt, drv_data);
>>> + platform_set_drvdata(pdev, drv_data);
>>> +
>>> + ret = watchdog_register_device(drv_data->wdt);
>>
>> Use devm_watchdog_register_device()
>>
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "failed to register Watchdog device\n");
>>> + return ret;
>>> + }
>>> +
>>> + ret = register_reboot_notifier(&ds1374_wdt_notifier);
>>
>> Call watchdog_stop_on_reboot() before calling watchdog_register_device()
>> instead.
>>
>>> + if (ret) {
>>> + watchdog_unregister_device(drv_data->wdt);
>>> + return ret;
>>> + }
>>> +
>>> + ds1374_wdt_settimeout(drv_data->wdt, wdt_margin);
>>
>> Unnecessary here; called from start function.
>>
>>> + dev_info(&pdev->dev, "Dallas/Maxim DS1374 watchdog device
>>> +enabled\n");
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ds1374_wdt_remove(struct platform_device *pdev) {
>>> + struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
>>> +
>>> + dev_warn(&pdev->dev, "Unregister DS1374 watchdog device\n");
>>> + watchdog_unregister_device(drv_data->wdt);
>>> + unregister_reboot_notifier(&ds1374_wdt_notifier);
>>> +
>>
>> Call watchdog_stop_on_unregister() before calling
>> watchdog_register_device().
>>
>>> + return 0;
>>> +}
>>> +
>>> +static void ds1374_wdt_shutdown(struct platform_device *pdev) {
>>> + struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
>>> +
>>> + mutex_lock(&drv_data->mutex);
>>> + ds1374_wdt_stop(drv_data->wdt);
>>> + mutex_unlock(&drv_data->mutex);
>>
>> Unnecessary and pointless.
>>
>>> +}
>>> +
>>> +static const struct watchdog_ops ds1374_wdt_fops = {
>>> + .owner = THIS_MODULE,
>>> + .start = ds1374_wdt_start,
>>> + .stop = ds1374_wdt_stop,
>>> + .ping = ds1374_wdt_ping,
>>> + .set_timeout = ds1374_wdt_settimeout,
>>> + .ioctl = ds1374_wdt_ioctl,
>>> +};
>>> +
>>> +static struct watchdog_device ds1374_wdd = {
>>> + .info = &ds1374_wdt_info,
>>> + .ops = &ds1374_wdt_fops,
>>> + .min_timeout = TIMER_MARGIN_MIN,
>>> + .max_timeout = TIMER_MARGIN_MAX,
>>
>> .timeout should be set here.
>>
>> Also, there can (at least in theory) be more than one ds1374 in the system. The
>> code does not support this case. ds1374_wdd should be moved into struct
>> ds1374_wdt.
>>
> Thanks for good suggestion.
>>> +};
>>> +
>>> +static struct platform_driver ds1374_wdt = {
>>> + .driver = {
>>> + .owner = THIS_MODULE,
>>> + .name = DEVNAME,
>>> + },
>>> + .probe = ds1374_wdt_probe,
>>> + .remove = ds1374_wdt_remove,
>>> + .shutdown = ds1374_wdt_shutdown,
>>> +};
>>> +
>>> +module_platform_driver(ds1374_wdt);
>>> +
>>> +MODULE_AUTHOR("Johnson Chen <johnsonch.chen@moxa.com>");
>>> +MODULE_DESCRIPTION("Dallas/Maxim DS1374 Watchdog Driver");
>>> +MODULE_LICENSE("GPL"); MODULE_ALIAS("Platform:ds1374_wdt");
>>>
>
> Best regards,
> Johnson
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver
2020-06-23 13:20 ` Guenter Roeck
@ 2020-06-24 12:09 ` Johnson CH Chen (陳昭勳)
0 siblings, 0 replies; 5+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-06-24 12:09 UTC (permalink / raw)
To: Guenter Roeck, linux-kernel
Cc: linux-rtc, linux-watchdog, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Lee Jones
Hi,
> On 6/22/20 11:28 PM, Johnson CH Chen (陳昭勳) wrote:
> > Hi,
> >
> > Thanks for your good detailed suggestions!
> >
>
> Other feedback suggests to convert the driver to use the watchdog core in the
> rtc code. I would suggest to follow that suggestion.
>
Understand the following suggestions for watchdog timer setting and I will improve them with watchdog core in rtc code later.
Best regards,
Johnson
> >>> + * It would be more efficient to use i2c msgs/i2c_transfer directly
> >>> +but, as
> >>> + * recommened in .../Documentation/i2c/writing-clients section
> >>> + * "Sending and receiving", using SMBus level communication is
> preferred.
> >>> + */
> >>> +
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/device.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/ioctl.h>
> >>> +#include <linux/reboot.h>
> >>> +#include <linux/watchdog.h>
> >>> +#include <linux/workqueue.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/i2c.h>
> >>> +#include <linux/uaccess.h>
> >>
> >> Alphabetic order please.
> >>
> >>> +
> >>> +#define DEVNAME "ds1374_wdt"
> >>> +
> >>> +#define DS1374_REG_WDALM0 0x04 /* Watchdog/Alarm */
> >>> +#define DS1374_REG_WDALM1 0x05
> >>> +#define DS1374_REG_WDALM2 0x06
> >>> +#define DS1374_REG_CR 0x07 /* Control */
> >>> +#define DS1374_REG_CR_AIE 0x01 /* Alarm Int. Enable */
> >>> +#define DS1374_REG_CR_WDSTR 0x08 /* 1=INT, 0=RST */
> >>> +#define DS1374_REG_CR_WDALM 0x20 /* 1=Watchdog, 0=Alarm */
> >>> +#define DS1374_REG_CR_WACE 0x40 /* WD/Alarm counter enable */
> >>> +
> >>> +/* Default margin */
> >>> +#define WD_TIMO 131762
> >>> +#define TIMER_MARGIN_MIN 4096 /* 1s */
> >>> +#define TIMER_MARGIN_MAX (60*60*24*4096) /* one day */
> >>> +
> >>> +static int wdt_margin = WD_TIMO;
> >>
> >> Sjould not be pre-initialized. Also, 131762 isn't 32 seconds, it is
> >> 131,762 seconds.
> >>
> >
> > 131762 is 32 seconds actually because watchdog counter increases per
> > 1/4096 seconds for DS1374. If DS1374_REG_CR_WDALM is set to 0 (alarm),
> > alarm counter will increase per 1 second.
> >
>
> The watchdog core keeps timeouts in seconds. For the watchdog core,
> 131762 is 131,762 seconds. Your code assumes that the watchdog core
> does not care about / need to scale, which is wrong. Besides,
> MODULE_PARM_DESC below clearly states "Watchdog timeout _in seconds_"
> (emphasis added).
>
> >>> +module_param(wdt_margin, int, 0);
> >>> +MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds
> (default
> >>> +32s)");
> >>> +
> >>
> >> As a new driver, it would be better to just use the customary "timeout".
> >>
> >>> +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) ")");
> >>> +
> >>> +struct ds1374_wdt {
> >>> + struct i2c_client *client;
> >>> + struct watchdog_device *wdt;
> >>> + struct work_struct work;
> >>
> >> Not used.
> >>
> >>> +
> >>> + /* The mutex protects alarm operations, and prevents a race
> >>> + * between the enable_irq() in the workqueue and the free_irq()
> >>> + * in the remove function.
> >>> + */
> >>
> >> There is no workqueue here, and the remove function is not needed.
> >>
> >>> + struct mutex mutex;
> >>> +};
> >>> +
> >>> +static const struct watchdog_info ds1374_wdt_info = {
> >>> + .identity = DEVNAME,
> >>> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> >>> + WDIOF_MAGICCLOSE,
> >>> +};
> >>> +
> >>> +static struct watchdog_device ds1374_wdd;
> >>> +
> >> Move declaration please.
> >>
> >>> +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
> >>> + unsigned int timeout)
> >>> +{
> >>
> >> How is this synchronized against accesses by the RTC driver ?
> >>
> > By original design in rtc-ds1374.c, it seems no synchronized problem
> between
> > rtc and watchdog, but I think we can add mutex lock to deal with it.
> >
> The mutex is used there where needed.
>
> >>> + int ret = -ENOIOCTLCMD;
> >>
> >> Not an appropriate error code here.
> >>
> >>> + u8 buf[4];
> >>> + int cr, i;
> >>> + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> >>> +
> >>> + ret = cr = i2c_smbus_read_byte_data(drv_data->client,
> DS1374_REG_CR);
> >>> + if (ret < 0)
> >>> + goto out;
> >>
> >> "goto out;" is unnecessary. Just return the error. Same everywhere else
> below.
> >>
> >>> +
> >>> + /* Disable any existing watchdog/alarm before setting the new one
> */
> >>> + cr &= ~DS1374_REG_CR_WACE;
> >>> +
> >>> + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR,
> cr);
> >>> + if (ret < 0)
> >>> + goto out;
> >>> +
> >>> + /* Set new watchdog time */
> >>> + for (i = 0; i < 3; i++) {
> >>> + buf[i] = timeout & 0xff;
> >>> + timeout >>= 8;
> >>> + }
> >>
> >> The value passed to this function from the watchdog core is the timeout in
> >> seconds. I don't see a conversion to internal values here.
> >>
> >
> > For original design in rtc-ds1374.c, ds1374_wdt_settimeout () will call
> > ds1374_write_rtc() to write hardware register. To separate watchdog and rtc
> > functions, expand code from ds1374_write_rtc() here, and final buf[] values
> > will be written into hardware register for DS1374.
> >
>
> ds1374_wdt_settimeout() is an API function, It gets timeouts from the
> watchdog
> core in seconds. Those timeouts have to be converted to chip internal values
> in this function.
>
> >>> +
> >>> + ret = i2c_smbus_write_i2c_block_data(drv_data->client,
> >>> + DS1374_REG_WDALM0, 3, buf);
> >>> + if (ret) {
> >>> + pr_info("couldn't set new watchdog time\n");
> >>> + goto out;
> >>> + }
> >>
> >>> +
> >>> + /* Enable watchdog timer */
> >>> + cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
> >>> + cr &= ~DS1374_REG_CR_WDSTR;/* for RST PIN */
> >>> + cr &= ~DS1374_REG_CR_AIE;
> >>> +
> >>> + ret = i2c_smbus_write_byte_data(drv_data->client, DS1374_REG_CR,
> cr);
> >>> + if (ret < 0)
> >>> + goto out;
> >>> +
> >>> + return 0;
> >>
> >> Pointless. Just return ret.
> >>
> >> Also, this function needs to store the new timeout in struct
> watchdog_device.
> >>
> >>> +out:
> >>> + return ret;
> >>> +}
> >>> +
> >>> +
> >>> +/*
> >>> + * Read WD/Alarm counter to reload the watchdog timer. (ie, pat the
> >>> +watchdog) */ static int ds1374_wdt_ping(struct watchdog_device *wdt)
> >>> +{
> >>> + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> >>> + int ret;
> >>> + u8 buf[4];
> >>> +
> >>> + ret = i2c_smbus_read_i2c_block_data(drv_data->client,
> >>> + DS1374_REG_WDALM0, 3, buf);
> >>> +
> >>> + if (ret < 0 || ret < 3)
> >>> + pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret);
> >>> +
> >>
> >> This is not an info message, this is an error. Besides, it is just noise.
> >> Just return the error and drop the message.
> >>
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static int ds1374_wdt_start(struct watchdog_device *wdt) {
> >>> + int ret;
> >>> +
> >>> + ret = ds1374_wdt_settimeout(wdt, wdt_margin);
> >>
> >> This is wrong. The timeout may have been updated by userspace.
> >> It is inappropriate to change it back to the default here.
> >>
> > Thanks, I will keep in mind.
> >
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + ds1374_wdt_ping(wdt);
> >>
> >> Please do not ignore errors.
> >>
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int ds1374_wdt_stop(struct watchdog_device *wdt) {
> >>> + struct ds1374_wdt *drv_data = watchdog_get_drvdata(wdt);
> >>> + int cr;
> >>> +
> >>> + if (nowayout)
> >>> + return 0;
> >>
> >> Not needed.
> >>
> > Thanks!, it has been implemented in watchdog_stop().
> >
> >>> +
> >>> + cr = i2c_smbus_read_byte_data(drv_data->client, DS1374_REG_CR);
> >>> + /* Disable watchdog timer */
> >>> + cr &= ~DS1374_REG_CR_WACE;
> >>> +
> >>> + return i2c_smbus_write_byte_data(drv_data->client,
> DS1374_REG_CR,
> >>> +cr); }
> >>> +
> >>> +/*
> >>> + * Handle commands from user-space.
> >>> + */
> >>> +static long ds1374_wdt_ioctl(struct watchdog_device *wdt, unsigned int
> >> cmd,
> >>> + unsigned long arg)
> >>
> >> The whole point of using the watchdog subsystem is to not need a local ioctl
> >> function - and most definitely not one that duplicates watchdog core
> >> functionality.
> >>
> >>> +{
> >>> + int new_margin, options;
> >>> +
> >>> + switch (cmd) {
> >>> + case WDIOC_GETSUPPORT:
> >>> + return copy_to_user((struct watchdog_info __user *)arg,
> >>> + &ds1374_wdt_info, sizeof(ds1374_wdt_info)) ? -EFAULT : 0;
> >>> +
> >>> + case WDIOC_GETSTATUS:
> >>> + case WDIOC_GETBOOTSTATUS:
> >>> + return put_user(0, (int __user *)arg);
> >>> + case WDIOC_KEEPALIVE:
> >>> + ds1374_wdt_ping(wdt);
> >>> + return 0;
> >>> + case WDIOC_SETTIMEOUT:
> >>> + if (get_user(new_margin, (int __user *)arg))
> >>> + return -EFAULT;
> >>> +
> >>> + /* the hardware's tick rate is 4096 Hz, so
> >>> + * the counter value needs to be scaled accordingly
> >>> + */
> >>> + new_margin <<= 12;
> >>> + if (new_margin < 1 || new_margin > 16777216)
> >>> + return -EINVAL;
> >>> +
> >>> + wdt_margin = new_margin;
> >>> + ds1374_wdt_settimeout(wdt, new_margin);
> >>
> >> Is the idea here to bypass the watchdog subsystem's notion of keeping the
> >> timeout in seconds ? If so, that would be wrong and unacceptable.
> >>
> > It seems take care about 4096Hz for original design in rtc-ds1374.c. I think
> we
> > can just use ioctl() which watchdog core provides and input margin time
> > appropriately.
> >
> >
> >>> + ds1374_wdt_ping(wdt);
> >>> + fallthrough;
> >>> + case WDIOC_GETTIMEOUT:
> >>> + /* when returning ... inverse is true */
> >>> + return put_user((wdt_margin >> 12), (int __user *)arg);
> >>> + case WDIOC_SETOPTIONS:
> >>> + if (copy_from_user(&options, (int __user *)arg, sizeof(int)))
> >>> + return -EFAULT;
> >>> +
> >>> + if (options & WDIOS_DISABLECARD) {
> >>> + pr_info("disable watchdog\n");
> >>> + ds1374_wdt_stop(wdt);
> >>> + return 0;
> >>> + }
> >>> +
> >>> + if (options & WDIOS_ENABLECARD) {
> >>> + pr_info("enable watchdog\n");
> >>> + ds1374_wdt_settimeout(wdt, wdt_margin);
> >>> + ds1374_wdt_ping(wdt);
> >>> + return 0;
> >>> + }
> >>> + return -EINVAL;
> >>> + }
> >>> + return -ENOTTY;
> >>> +}
> >>> +
> >>> +static int ds1374_wdt_notify_sys(struct notifier_block *this,
> >>> + unsigned long code, void *unused)
> >>> +{
> >>> + if (code == SYS_DOWN || code == SYS_HALT)
> >>> + /* Disable Watchdog */
> >>> + ds1374_wdt_stop(&ds1374_wdd);
> >>> + return NOTIFY_DONE;
> >>> +}
> >>> +
> >> Not needed - see below.
> >>
> >>> +static struct notifier_block ds1374_wdt_notifier = {
> >>> + .notifier_call = ds1374_wdt_notify_sys, };
> >>> +
> >>> +static int ds1374_wdt_probe(struct platform_device *pdev) {
> >>> + struct ds1374_wdt *drv_data;
> >>> + struct i2c_client *client = to_i2c_client(pdev->dev.parent);
> >>> + int ret;
> >>> +
> >>> + drv_data = devm_kzalloc(&pdev->dev, sizeof(struct ds1374_wdt),
> >>> + GFP_KERNEL);
> >>> + if (!drv_data)
> >>> + return -ENOMEM;
> >>> +
> >>> + drv_data->wdt = &ds1374_wdd;
> >>> + drv_data->client = client;
> >>> + mutex_init(&drv_data->mutex);
> >>> +
> >>> + watchdog_init_timeout(drv_data->wdt, wdt_margin, &pdev->dev);
> >>> + watchdog_set_nowayout(drv_data->wdt, nowayout);
> >>> +
> >>> + watchdog_set_drvdata(drv_data->wdt, drv_data);
> >>> + platform_set_drvdata(pdev, drv_data);
> >>> +
> >>> + ret = watchdog_register_device(drv_data->wdt);
> >>
> >> Use devm_watchdog_register_device()
> >>
> >>> + if (ret) {
> >>> + dev_err(&pdev->dev, "failed to register Watchdog device\n");
> >>> + return ret;
> >>> + }
> >>> +
> >>> + ret = register_reboot_notifier(&ds1374_wdt_notifier);
> >>
> >> Call watchdog_stop_on_reboot() before calling watchdog_register_device()
> >> instead.
> >>
> >>> + if (ret) {
> >>> + watchdog_unregister_device(drv_data->wdt);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + ds1374_wdt_settimeout(drv_data->wdt, wdt_margin);
> >>
> >> Unnecessary here; called from start function.
> >>
> >>> + dev_info(&pdev->dev, "Dallas/Maxim DS1374 watchdog device
> >>> +enabled\n");
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int ds1374_wdt_remove(struct platform_device *pdev) {
> >>> + struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
> >>> +
> >>> + dev_warn(&pdev->dev, "Unregister DS1374 watchdog device\n");
> >>> + watchdog_unregister_device(drv_data->wdt);
> >>> + unregister_reboot_notifier(&ds1374_wdt_notifier);
> >>> +
> >>
> >> Call watchdog_stop_on_unregister() before calling
> >> watchdog_register_device().
> >>
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static void ds1374_wdt_shutdown(struct platform_device *pdev) {
> >>> + struct ds1374_wdt *drv_data = platform_get_drvdata(pdev);
> >>> +
> >>> + mutex_lock(&drv_data->mutex);
> >>> + ds1374_wdt_stop(drv_data->wdt);
> >>> + mutex_unlock(&drv_data->mutex);
> >>
> >> Unnecessary and pointless.
> >>
> >>> +}
> >>> +
> >>> +static const struct watchdog_ops ds1374_wdt_fops = {
> >>> + .owner = THIS_MODULE,
> >>> + .start = ds1374_wdt_start,
> >>> + .stop = ds1374_wdt_stop,
> >>> + .ping = ds1374_wdt_ping,
> >>> + .set_timeout = ds1374_wdt_settimeout,
> >>> + .ioctl = ds1374_wdt_ioctl,
> >>> +};
> >>> +
> >>> +static struct watchdog_device ds1374_wdd = {
> >>> + .info = &ds1374_wdt_info,
> >>> + .ops = &ds1374_wdt_fops,
> >>> + .min_timeout = TIMER_MARGIN_MIN,
> >>> + .max_timeout = TIMER_MARGIN_MAX,
> >>
> >> .timeout should be set here.
> >>
> >> Also, there can (at least in theory) be more than one ds1374 in the system.
> The
> >> code does not support this case. ds1374_wdd should be moved into struct
> >> ds1374_wdt.
> >>
> > Thanks for good suggestion.
> >>> +};
> >>> +
> >>> +static struct platform_driver ds1374_wdt = {
> >>> + .driver = {
> >>> + .owner = THIS_MODULE,
> >>> + .name = DEVNAME,
> >>> + },
> >>> + .probe = ds1374_wdt_probe,
> >>> + .remove = ds1374_wdt_remove,
> >>> + .shutdown = ds1374_wdt_shutdown,
> >>> +};
> >>> +
> >>> +module_platform_driver(ds1374_wdt);
> >>> +
> >>> +MODULE_AUTHOR("Johnson Chen <johnsonch.chen@moxa.com>");
> >>> +MODULE_DESCRIPTION("Dallas/Maxim DS1374 Watchdog Driver");
> >>> +MODULE_LICENSE("GPL"); MODULE_ALIAS("Platform:ds1374_wdt");
> >>>
> >
> > Best regards,
> > Johnson
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-24 12:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-22 10:03 [PATCH 3/3] watchdog: ds1374_wdt: Introduce Dallas/Maxim DS1374 Watchdog driver Johnson CH Chen (陳昭勳)
2020-06-22 14:26 ` Guenter Roeck
2020-06-23 6:28 ` Johnson CH Chen (陳昭勳)
2020-06-23 13:20 ` Guenter Roeck
2020-06-24 12:09 ` Johnson CH Chen (陳昭勳)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).