Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part
@ 2020-07-01 10:09 Johnson CH Chen (陳昭勳)
  2020-07-01 14:05 ` Guenter Roeck
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-07-01 10:09 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-rtc, LINUX-WATCHDOG, Wim Van Sebroeck, Alessandro Zummo,
	Alexandre Belloni, Guenter Roeck

Let ds1374 watchdog use watchdog core. It also includes
improving watchdog timer setting and nowayout, and just uses ioctl()
of watchdog core.

Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
---
 drivers/rtc/rtc-ds1374.c | 257 ++++++++++-----------------------------
 1 file changed, 67 insertions(+), 190 deletions(-)

diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 9c51a12cf70f..25b28f7546ff 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * RTC client/driver for the Maxim/Dallas DS1374 Real-Time Clock over I2C
  *
@@ -6,11 +7,7 @@
  *
  * Copyright (C) 2014 Rose Technology
  * Copyright (C) 2006-2007 Freescale Semiconductor
- *
- * 2005 (c) MontaVista Software, Inc. This file is licensed under
- * the terms of the GNU General Public License version 2. This program
- * is licensed "as is" without any warranty of any kind, whether express
- * or implied.
+ * Copyright (C) 2005 (c) MontaVista Software, Inc.
  */
 /*
  * It would be more efficient to use i2c msgs/i2c_transfer directly but, as
@@ -46,6 +43,7 @@
 #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 */
 #define DS1374_REG_SR		0x08 /* Status */
@@ -71,7 +69,9 @@ struct ds1374 {
 	struct i2c_client *client;
 	struct rtc_device *rtc;
 	struct work_struct work;
-
+#ifdef CONFIG_RTC_DRV_DS1374_WDT
+	struct watchdog_device wdt;
+#endif
 	/* The mutex protects alarm operations, and prevents a race
 	 * between the enable_irq() in the workqueue and the free_irq()
 	 * in the remove function.
@@ -257,7 +257,8 @@ static int ds1374_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 		goto out;
 
 	/* Disable any existing alarm before setting the new one
-	 * (or lack thereof). */
+	 * (or lack thereof).
+	 */
 	cr &= ~DS1374_REG_CR_WACE;
 
 	ret = i2c_smbus_write_byte_data(client, DS1374_REG_CR, cr);
@@ -371,14 +372,21 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
  */
 static struct i2c_client *save_client;
 /* Default margin */
-#define WD_TIMO 131762
+#define TIMER_MARGIN_DEFAULT  32
+#define TIMER_MARGIN_MIN	1
+#define TIMER_MARGIN_MAX	(60*60*24) /* one day */
 
 #define DRV_NAME "DS1374 Watchdog"
 
-static int wdt_margin = WD_TIMO;
-static unsigned long wdt_is_open;
-module_param(wdt_margin, int, 0);
-MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 32s)");
+static int timeout = TIMER_MARGIN_DEFAULT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "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)")");
+
 
 static const struct watchdog_info ds1374_wdt_info = {
 	.identity       = "DS1374 WTD",
@@ -386,57 +394,61 @@ static const struct watchdog_info ds1374_wdt_info = {
 						WDIOF_MAGICCLOSE,
 };
 
-static int ds1374_wdt_settimeout(unsigned int timeout)
+static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
+					unsigned int timeout)
 {
-	int ret = -ENOIOCTLCMD;
-	int cr;
+	int ret, cr;
+
+	wdt->timeout = timeout;
 
-	ret = cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
+	cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
+	ret = cr;
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	/* Disable any existing watchdog/alarm before setting the new one */
 	cr &= ~DS1374_REG_CR_WACE;
 
 	ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	/* Set new watchdog time */
+	timeout = timeout * 4096;
 	ret = ds1374_write_rtc(save_client, timeout, DS1374_REG_WDALM0, 3);
 	if (ret) {
 		pr_info("couldn't set new watchdog time\n");
-		goto out;
+		return ret;
 	}
 
 	/* 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(save_client, DS1374_REG_CR, cr);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	return 0;
-out:
-	return ret;
 }
 
-
 /*
  * Reload the watchdog timer.  (ie, pat the watchdog)
  */
-static void ds1374_wdt_ping(void)
+static int ds1374_wdt_ping(struct watchdog_device *wdt)
 {
 	u32 val;
-	int ret = 0;
 
-	ret = ds1374_read_rtc(save_client, &val, DS1374_REG_WDALM0, 3);
-	if (ret)
-		pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret);
+	return ds1374_read_rtc(save_client, &val, DS1374_REG_WDALM0, 3);
+}
+
+static int ds1374_wdt_start(struct watchdog_device *wdt)
+{
+	return ds1374_wdt_ping(wdt);
 }
 
-static void ds1374_wdt_disable(void)
+static int ds1374_wdt_stop(struct watchdog_device *wdt)
 {
 	int cr;
 
@@ -444,162 +456,17 @@ static void ds1374_wdt_disable(void)
 	/* Disable watchdog timer */
 	cr &= ~DS1374_REG_CR_WACE;
 
-	i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
+	return i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
 }
 
-/*
- * Watchdog device is opened, and watchdog starts running.
- */
-static int ds1374_wdt_open(struct inode *inode, struct file *file)
-{
-	struct ds1374 *ds1374 = i2c_get_clientdata(save_client);
-
-	if (MINOR(inode->i_rdev) == WATCHDOG_MINOR) {
-		mutex_lock(&ds1374->mutex);
-		if (test_and_set_bit(0, &wdt_is_open)) {
-			mutex_unlock(&ds1374->mutex);
-			return -EBUSY;
-		}
-		/*
-		 *      Activate
-		 */
-		wdt_is_open = 1;
-		mutex_unlock(&ds1374->mutex);
-		return stream_open(inode, file);
-	}
-	return -ENODEV;
-}
-
-/*
- * Close the watchdog device.
- */
-static int ds1374_wdt_release(struct inode *inode, struct file *file)
-{
-	if (MINOR(inode->i_rdev) == WATCHDOG_MINOR)
-		clear_bit(0, &wdt_is_open);
-
-	return 0;
-}
-
-/*
- * Pat the watchdog whenever device is written to.
- */
-static ssize_t ds1374_wdt_write(struct file *file, const char __user *data,
-				size_t len, loff_t *ppos)
-{
-	if (len) {
-		ds1374_wdt_ping();
-		return 1;
-	}
-	return 0;
-}
-
-static ssize_t ds1374_wdt_read(struct file *file, char __user *data,
-				size_t len, loff_t *ppos)
-{
-	return 0;
-}
-
-/*
- * Handle commands from user-space.
- */
-static long ds1374_wdt_ioctl(struct file *file, 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();
-		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(new_margin);
-		ds1374_wdt_ping();
-		/* 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_disable();
-			return 0;
-		}
-
-		if (options & WDIOS_ENABLECARD) {
-			pr_info("enable watchdog\n");
-			ds1374_wdt_settimeout(wdt_margin);
-			ds1374_wdt_ping();
-			return 0;
-		}
-		return -EINVAL;
-	}
-	return -ENOTTY;
-}
-
-static long ds1374_wdt_unlocked_ioctl(struct file *file, unsigned int cmd,
-			unsigned long arg)
-{
-	int ret;
-	struct ds1374 *ds1374 = i2c_get_clientdata(save_client);
-
-	mutex_lock(&ds1374->mutex);
-	ret = ds1374_wdt_ioctl(file, cmd, arg);
-	mutex_unlock(&ds1374->mutex);
-
-	return ret;
-}
-
-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_disable();
-	return NOTIFY_DONE;
-}
-
-static const struct file_operations ds1374_wdt_fops = {
-	.owner			= THIS_MODULE,
-	.read			= ds1374_wdt_read,
-	.unlocked_ioctl		= ds1374_wdt_unlocked_ioctl,
-	.compat_ioctl		= compat_ptr_ioctl,
-	.write			= ds1374_wdt_write,
-	.open                   = ds1374_wdt_open,
-	.release                = ds1374_wdt_release,
-	.llseek			= no_llseek,
-};
-
-static struct miscdevice ds1374_miscdev = {
-	.minor          = WATCHDOG_MINOR,
-	.name           = "watchdog",
-	.fops           = &ds1374_wdt_fops,
+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,
 };
 
-static struct notifier_block ds1374_wdt_notifier = {
-	.notifier_call = ds1374_wdt_notify_sys,
-};
 
 #endif /*CONFIG_RTC_DRV_DS1374_WDT*/
 /*
@@ -653,15 +520,25 @@ static int ds1374_probe(struct i2c_client *client,
 
 #ifdef CONFIG_RTC_DRV_DS1374_WDT
 	save_client = client;
-	ret = misc_register(&ds1374_miscdev);
-	if (ret)
-		return ret;
-	ret = register_reboot_notifier(&ds1374_wdt_notifier);
+	ds1374->wdt.info = &ds1374_wdt_info;
+	ds1374->wdt.ops = &ds1374_wdt_fops;
+	ds1374->wdt.timeout = TIMER_MARGIN_DEFAULT;
+	ds1374->wdt.min_timeout = TIMER_MARGIN_MIN;
+	ds1374->wdt.max_timeout = TIMER_MARGIN_MAX;
+
+	watchdog_init_timeout(&ds1374->wdt, timeout, &client->dev);
+	watchdog_set_nowayout(&ds1374->wdt, nowayout);
+	watchdog_stop_on_reboot(&ds1374->wdt);
+	watchdog_stop_on_unregister(&ds1374->wdt);
+
+	ret = devm_watchdog_register_device(&client->dev, &ds1374->wdt);
 	if (ret) {
-		misc_deregister(&ds1374_miscdev);
+		dev_err(&client->dev, "failed to register DS1374 watchdog device\n");
 		return ret;
 	}
-	ds1374_wdt_settimeout(131072);
+
+	ds1374_wdt_settimeout(&ds1374->wdt, timeout);
+	dev_info(&client->dev, "DS1374 watchdog device enabled\n");
 #endif
 
 	return 0;
@@ -670,10 +547,10 @@ static int ds1374_probe(struct i2c_client *client,
 static int ds1374_remove(struct i2c_client *client)
 {
 	struct ds1374 *ds1374 = i2c_get_clientdata(client);
+
 #ifdef CONFIG_RTC_DRV_DS1374_WDT
-	misc_deregister(&ds1374_miscdev);
-	ds1374_miscdev.parent = NULL;
-	unregister_reboot_notifier(&ds1374_wdt_notifier);
+	dev_warn(&client->dev, "Unregister DS1374 watchdog device\n");
+	watchdog_unregister_device(&ds1374->wdt);
 #endif
 
 	if (client->irq > 0) {
-- 
2.20.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part
  2020-07-01 10:09 [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part Johnson CH Chen (陳昭勳)
@ 2020-07-01 14:05 ` Guenter Roeck
  2020-07-03 10:15   ` Johnson CH Chen (陳昭勳)
  2020-07-02 12:25 ` kernel test robot
  2020-07-02 14:38 ` kernel test robot
  2 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2020-07-01 14:05 UTC (permalink / raw)
  To: Johnson CH Chen (陳昭勳), linux-kernel
  Cc: linux-rtc, LINUX-WATCHDOG, Wim Van Sebroeck, Alessandro Zummo,
	Alexandre Belloni

On 7/1/20 3:09 AM, Johnson CH Chen (陳昭勳) wrote:
> Let ds1374 watchdog use watchdog core. It also includes
> improving watchdog timer setting and nowayout, and just uses ioctl()
> of watchdog core.
> 
> Signed-off-by: Johnson Chen <johnsonch.chen@moxa.com>
> ---
>  drivers/rtc/rtc-ds1374.c | 257 ++++++++++-----------------------------
>  1 file changed, 67 insertions(+), 190 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
> index 9c51a12cf70f..25b28f7546ff 100644
> --- a/drivers/rtc/rtc-ds1374.c
> +++ b/drivers/rtc/rtc-ds1374.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * RTC client/driver for the Maxim/Dallas DS1374 Real-Time Clock over I2C
>   *
> @@ -6,11 +7,7 @@
>   *
>   * Copyright (C) 2014 Rose Technology
>   * Copyright (C) 2006-2007 Freescale Semiconductor
> - *
> - * 2005 (c) MontaVista Software, Inc. This file is licensed under
> - * the terms of the GNU General Public License version 2. This program
> - * is licensed "as is" without any warranty of any kind, whether express
> - * or implied.
> + * Copyright (C) 2005 (c) MontaVista Software, Inc.
>   */

The above should be a separate patch.

>  /*
>   * It would be more efficient to use i2c msgs/i2c_transfer directly but, as
> @@ -46,6 +43,7 @@
>  #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 */
>  #define DS1374_REG_SR		0x08 /* Status */
> @@ -71,7 +69,9 @@ struct ds1374 {
>  	struct i2c_client *client;
>  	struct rtc_device *rtc;
>  	struct work_struct work;
> -
> +#ifdef CONFIG_RTC_DRV_DS1374_WDT
> +	struct watchdog_device wdt;
> +#endif
>  	/* The mutex protects alarm operations, and prevents a race
>  	 * between the enable_irq() in the workqueue and the free_irq()
>  	 * in the remove function.
> @@ -257,7 +257,8 @@ static int ds1374_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>  		goto out;
>  
>  	/* Disable any existing alarm before setting the new one
> -	 * (or lack thereof). */
> +	 * (or lack thereof).
> +	 */

Unrelated (and cosmetic). Drop or separate patch.

>  	cr &= ~DS1374_REG_CR_WACE;
>  
>  	ret = i2c_smbus_write_byte_data(client, DS1374_REG_CR, cr);
> @@ -371,14 +372,21 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
>   */
>  static struct i2c_client *save_client;
>  /* Default margin */
> -#define WD_TIMO 131762
> +#define TIMER_MARGIN_DEFAULT  32

Parameter alignment is off. Please use tab after TIMER_MARGIN_DEFAULT.

> +#define TIMER_MARGIN_MIN	1
> +#define TIMER_MARGIN_MAX	(60*60*24) /* one day */

Real limits are necessary. The old limit was 16777216/4096 = 4096 seconds.

Ok, time to look up the datasheet. The counter is a 24-bit value.
The maximum value is thus 0xffffff, or 16777215. 16777215 / 4096 = 4095,
which should be the maximum timeout in seconds to set here.

>  
>  #define DRV_NAME "DS1374 Watchdog"
>  
> -static int wdt_margin = WD_TIMO;
> -static unsigned long wdt_is_open;
> -module_param(wdt_margin, int, 0);
> -MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default 32s)");
> +static int timeout = TIMER_MARGIN_DEFAULT;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default 32s)");

This changes the module parameter which may be unexpected.
I would suggest to keep using wdt_margin.

> +
> +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 const struct watchdog_info ds1374_wdt_info = {
>  	.identity       = "DS1374 WTD",
> @@ -386,57 +394,61 @@ static const struct watchdog_info ds1374_wdt_info = {
>  						WDIOF_MAGICCLOSE,
>  };
>  
> -static int ds1374_wdt_settimeout(unsigned int timeout)
> +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
> +					unsigned int timeout)

This fits into one line (new line length limit)

>  {
> -	int ret = -ENOIOCTLCMD;
> -	int cr;
> +	int ret, cr;
> +
> +	wdt->timeout = timeout;
>  
> -	ret = cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
> +	cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
> +	ret = cr;
>  	if (ret < 0)
> -		goto out;
> +		return ret;
>  
Assignment to ret is pointless. Just return cr if negative.

>  	/* Disable any existing watchdog/alarm before setting the new one */
>  	cr &= ~DS1374_REG_CR_WACE;
>  
>  	ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
>  	if (ret < 0)
> -		goto out;
> +		return ret;
>  
>  	/* Set new watchdog time */
> +	timeout = timeout * 4096;
>  	ret = ds1374_write_rtc(save_client, timeout, DS1374_REG_WDALM0, 3);
>  	if (ret) {
>  		pr_info("couldn't set new watchdog time\n");
> -		goto out;
> +		return ret;
>  	}
>  
>  	/* 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(save_client, DS1374_REG_CR, cr);
>  	if (ret < 0)
> -		goto out;
> +		return ret;
>  
>  	return 0;
> -out:
> -	return ret;
>  }
>  
> -
>  /*
>   * Reload the watchdog timer.  (ie, pat the watchdog)
>   */
> -static void ds1374_wdt_ping(void)
> +static int ds1374_wdt_ping(struct watchdog_device *wdt)
>  {
>  	u32 val;
> -	int ret = 0;
>  
> -	ret = ds1374_read_rtc(save_client, &val, DS1374_REG_WDALM0, 3);
> -	if (ret)
> -		pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret);
> +	return ds1374_read_rtc(save_client, &val, DS1374_REG_WDALM0, 3);
> +}
> +
> +static int ds1374_wdt_start(struct watchdog_device *wdt)
> +{
> +	return ds1374_wdt_ping(wdt);

Unnecessary. Just declare the start function and drop the ping function.
See Documentation/watchdog/watchdog-kernel-api.rst

>  }
>  
> -static void ds1374_wdt_disable(void)
> +static int ds1374_wdt_stop(struct watchdog_device *wdt)
>  {
>  	int cr;
>  
> @@ -444,162 +456,17 @@ static void ds1374_wdt_disable(void)
>  	/* Disable watchdog timer */
>  	cr &= ~DS1374_REG_CR_WACE;
>  
> -	i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
> +	return i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
>  }
>  
> -/*
> - * Watchdog device is opened, and watchdog starts running.
> - */
> -static int ds1374_wdt_open(struct inode *inode, struct file *file)
> -{
> -	struct ds1374 *ds1374 = i2c_get_clientdata(save_client);
> -
> -	if (MINOR(inode->i_rdev) == WATCHDOG_MINOR) {
> -		mutex_lock(&ds1374->mutex);
> -		if (test_and_set_bit(0, &wdt_is_open)) {
> -			mutex_unlock(&ds1374->mutex);
> -			return -EBUSY;
> -		}
> -		/*
> -		 *      Activate
> -		 */
> -		wdt_is_open = 1;
> -		mutex_unlock(&ds1374->mutex);
> -		return stream_open(inode, file);
> -	}
> -	return -ENODEV;
> -}
> -
> -/*
> - * Close the watchdog device.
> - */
> -static int ds1374_wdt_release(struct inode *inode, struct file *file)
> -{
> -	if (MINOR(inode->i_rdev) == WATCHDOG_MINOR)
> -		clear_bit(0, &wdt_is_open);
> -
> -	return 0;
> -}
> -
> -/*
> - * Pat the watchdog whenever device is written to.
> - */
> -static ssize_t ds1374_wdt_write(struct file *file, const char __user *data,
> -				size_t len, loff_t *ppos)
> -{
> -	if (len) {
> -		ds1374_wdt_ping();
> -		return 1;
> -	}
> -	return 0;
> -}
> -
> -static ssize_t ds1374_wdt_read(struct file *file, char __user *data,
> -				size_t len, loff_t *ppos)
> -{
> -	return 0;
> -}
> -
> -/*
> - * Handle commands from user-space.
> - */
> -static long ds1374_wdt_ioctl(struct file *file, 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();
> -		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(new_margin);
> -		ds1374_wdt_ping();
> -		/* 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_disable();
> -			return 0;
> -		}
> -
> -		if (options & WDIOS_ENABLECARD) {
> -			pr_info("enable watchdog\n");
> -			ds1374_wdt_settimeout(wdt_margin);
> -			ds1374_wdt_ping();
> -			return 0;
> -		}
> -		return -EINVAL;
> -	}
> -	return -ENOTTY;
> -}
> -
> -static long ds1374_wdt_unlocked_ioctl(struct file *file, unsigned int cmd,
> -			unsigned long arg)
> -{
> -	int ret;
> -	struct ds1374 *ds1374 = i2c_get_clientdata(save_client);
> -
> -	mutex_lock(&ds1374->mutex);
> -	ret = ds1374_wdt_ioctl(file, cmd, arg);
> -	mutex_unlock(&ds1374->mutex);
> -
> -	return ret;
> -}
> -
> -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_disable();
> -	return NOTIFY_DONE;
> -}
> -
> -static const struct file_operations ds1374_wdt_fops = {
> -	.owner			= THIS_MODULE,
> -	.read			= ds1374_wdt_read,
> -	.unlocked_ioctl		= ds1374_wdt_unlocked_ioctl,
> -	.compat_ioctl		= compat_ptr_ioctl,
> -	.write			= ds1374_wdt_write,
> -	.open                   = ds1374_wdt_open,
> -	.release                = ds1374_wdt_release,
> -	.llseek			= no_llseek,
> -};
> -
> -static struct miscdevice ds1374_miscdev = {
> -	.minor          = WATCHDOG_MINOR,
> -	.name           = "watchdog",
> -	.fops           = &ds1374_wdt_fops,
> +static const struct watchdog_ops ds1374_wdt_fops = {

s/fops/ops/

Those are not file operations.

> +	.owner          = THIS_MODULE,
> +	.start          = ds1374_wdt_start,
> +	.stop           = ds1374_wdt_stop,
> +	.ping           = ds1374_wdt_ping,
> +	.set_timeout    = ds1374_wdt_settimeout,
>  };
>  
> -static struct notifier_block ds1374_wdt_notifier = {
> -	.notifier_call = ds1374_wdt_notify_sys,
> -};
>  
>  #endif /*CONFIG_RTC_DRV_DS1374_WDT*/
>  /*
> @@ -653,15 +520,25 @@ static int ds1374_probe(struct i2c_client *client,
>  
>  #ifdef CONFIG_RTC_DRV_DS1374_WDT
>  	save_client = client;
> -	ret = misc_register(&ds1374_miscdev);
> -	if (ret)
> -		return ret;
> -	ret = register_reboot_notifier(&ds1374_wdt_notifier);
> +	ds1374->wdt.info = &ds1374_wdt_info;
> +	ds1374->wdt.ops = &ds1374_wdt_fops;
> +	ds1374->wdt.timeout = TIMER_MARGIN_DEFAULT;
> +	ds1374->wdt.min_timeout = TIMER_MARGIN_MIN;
> +	ds1374->wdt.max_timeout = TIMER_MARGIN_MAX;
> +
> +	watchdog_init_timeout(&ds1374->wdt, timeout, &client->dev);
> +	watchdog_set_nowayout(&ds1374->wdt, nowayout);
> +	watchdog_stop_on_reboot(&ds1374->wdt);
> +	watchdog_stop_on_unregister(&ds1374->wdt);
> +
> +	ret = devm_watchdog_register_device(&client->dev, &ds1374->wdt);
>  	if (ret) {
> -		misc_deregister(&ds1374_miscdev);
> +		dev_err(&client->dev, "failed to register DS1374 watchdog device\n");
>  		return ret;
>  	}
> -	ds1374_wdt_settimeout(131072);
> +
> +	ds1374_wdt_settimeout(&ds1374->wdt, timeout);

You'll want to do that before registering the watchdog to avoid a
race condition.

> +	dev_info(&client->dev, "DS1374 watchdog device enabled\n");
>  #endif
>  
>  	return 0;
> @@ -670,10 +547,10 @@ static int ds1374_probe(struct i2c_client *client,
>  static int ds1374_remove(struct i2c_client *client)
>  {
>  	struct ds1374 *ds1374 = i2c_get_clientdata(client);
> +
>  #ifdef CONFIG_RTC_DRV_DS1374_WDT
> -	misc_deregister(&ds1374_miscdev);
> -	ds1374_miscdev.parent = NULL;
> -	unregister_reboot_notifier(&ds1374_wdt_notifier);
> +	dev_warn(&client->dev, "Unregister DS1374 watchdog device\n");

Noise.

> +	watchdog_unregister_device(&ds1374->wdt);

Drop: The watchdog was registered using devm_watchdog_register_device().

>  #endif
>  
>  	if (client->irq > 0) {
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part
  2020-07-01 10:09 [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part Johnson CH Chen (陳昭勳)
  2020-07-01 14:05 ` Guenter Roeck
@ 2020-07-02 12:25 ` kernel test robot
  2020-07-02 14:38 ` kernel test robot
  2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2020-07-02 12:25 UTC (permalink / raw)
  To: Johnson CH Chen (陳昭勳), linux-kernel
  Cc: kbuild-all, linux-rtc, LINUX-WATCHDOG, Wim Van Sebroeck,
	Alessandro Zummo, Alexandre Belloni, Guenter Roeck


[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

Hi "Johnson,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on abelloni/rtc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Johnson-CH-Chen/rtc-ds1374-wdt-Use-watchdog-core-for-watchdog-part/20200701-181230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: x86_64-randconfig-a015-20200701 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "devm_watchdog_register_device" [drivers/rtc/rtc-ds1374.ko] undefined!
>> ERROR: modpost: "watchdog_init_timeout" [drivers/rtc/rtc-ds1374.ko] undefined!
>> ERROR: modpost: "watchdog_unregister_device" [drivers/rtc/rtc-ds1374.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31996 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part
  2020-07-01 10:09 [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part Johnson CH Chen (陳昭勳)
  2020-07-01 14:05 ` Guenter Roeck
  2020-07-02 12:25 ` kernel test robot
@ 2020-07-02 14:38 ` kernel test robot
  2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2020-07-02 14:38 UTC (permalink / raw)
  To: Johnson CH Chen (陳昭勳), linux-kernel
  Cc: kbuild-all, linux-rtc, LINUX-WATCHDOG, Wim Van Sebroeck,
	Alessandro Zummo, Alexandre Belloni, Guenter Roeck


[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

Hi "Johnson,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on abelloni/rtc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Johnson-CH-Chen/rtc-ds1374-wdt-Use-watchdog-core-for-watchdog-part/20200701-181230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: i386-randconfig-a006-20200701 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: drivers/rtc/rtc-ds1374.o: in function `ds1374_probe':
>> rtc-ds1374.c:(.text+0x41c): undefined reference to `watchdog_init_timeout'
>> ld: rtc-ds1374.c:(.text+0x443): undefined reference to `devm_watchdog_register_device'
   ld: drivers/rtc/rtc-ds1374.o: in function `ds1374_remove':
>> rtc-ds1374.c:(.text.unlikely+0x1e): undefined reference to `watchdog_unregister_device'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35591 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part
  2020-07-01 14:05 ` Guenter Roeck
@ 2020-07-03 10:15   ` Johnson CH Chen (陳昭勳)
  0 siblings, 0 replies; 5+ messages in thread
From: Johnson CH Chen (陳昭勳) @ 2020-07-03 10:15 UTC (permalink / raw)
  To: Guenter Roeck, linux-kernel
  Cc: linux-rtc, LINUX-WATCHDOG, Wim Van Sebroeck, Alessandro Zummo,
	Alexandre Belloni

Hi,

> > diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c index
> > 9c51a12cf70f..25b28f7546ff 100644
> > --- a/drivers/rtc/rtc-ds1374.c
> > +++ b/drivers/rtc/rtc-ds1374.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> >  /*
> >   * RTC client/driver for the Maxim/Dallas DS1374 Real-Time Clock over
> I2C
> >   *
> > @@ -6,11 +7,7 @@
> >   *
> >   * Copyright (C) 2014 Rose Technology
> >   * Copyright (C) 2006-2007 Freescale Semiconductor
> > - *
> > - * 2005 (c) MontaVista Software, Inc. This file is licensed under
> > - * the terms of the GNU General Public License version 2. This
> > program
> > - * is licensed "as is" without any warranty of any kind, whether
> > express
> > - * or implied.
> > + * Copyright (C) 2005 (c) MontaVista Software, Inc.
> >   */
> 
> The above should be a separate patch.
> 
> >  /*
> >   * It would be more efficient to use i2c msgs/i2c_transfer directly
> > but, as @@ -46,6 +43,7 @@
> >  #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 */
> >  #define DS1374_REG_SR		0x08 /* Status */
> > @@ -71,7 +69,9 @@ struct ds1374 {
> >  	struct i2c_client *client;
> >  	struct rtc_device *rtc;
> >  	struct work_struct work;
> > -
> > +#ifdef CONFIG_RTC_DRV_DS1374_WDT
> > +	struct watchdog_device wdt;
> > +#endif
> >  	/* The mutex protects alarm operations, and prevents a race
> >  	 * between the enable_irq() in the workqueue and the free_irq()
> >  	 * in the remove function.
> > @@ -257,7 +257,8 @@ static int ds1374_set_alarm(struct device *dev,
> struct rtc_wkalrm *alarm)
> >  		goto out;
> >
> >  	/* Disable any existing alarm before setting the new one
> > -	 * (or lack thereof). */
> > +	 * (or lack thereof).
> > +	 */
> 
> Unrelated (and cosmetic). Drop or separate patch.
> 
> >  	cr &= ~DS1374_REG_CR_WACE;
> >
> >  	ret = i2c_smbus_write_byte_data(client, DS1374_REG_CR, cr); @@
> > -371,14 +372,21 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
> >   */
> >  static struct i2c_client *save_client;
> >  /* Default margin */
> > -#define WD_TIMO 131762
> > +#define TIMER_MARGIN_DEFAULT  32
> 
> Parameter alignment is off. Please use tab after TIMER_MARGIN_DEFAULT.
> 
> > +#define TIMER_MARGIN_MIN	1
> > +#define TIMER_MARGIN_MAX	(60*60*24) /* one day */
> 
> Real limits are necessary. The old limit was 16777216/4096 = 4096 seconds.
> 
> Ok, time to look up the datasheet. The counter is a 24-bit value.
> The maximum value is thus 0xffffff, or 16777215. 16777215 / 4096 = 4095,
> which should be the maximum timeout in seconds to set here.
> 
> >
> >  #define DRV_NAME "DS1374 Watchdog"
> >
> > -static int wdt_margin = WD_TIMO;
> > -static unsigned long wdt_is_open;
> > -module_param(wdt_margin, int, 0);
> > -MODULE_PARM_DESC(wdt_margin, "Watchdog timeout in seconds (default
> > 32s)");
> > +static int timeout = TIMER_MARGIN_DEFAULT; module_param(timeout, int,
> > +0); MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default
> > +32s)");
> 
> This changes the module parameter which may be unexpected.
> I would suggest to keep using wdt_margin.
> 
> > +
> > +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 const struct watchdog_info ds1374_wdt_info = {
> >  	.identity       = "DS1374 WTD",
> > @@ -386,57 +394,61 @@ static const struct watchdog_info
> ds1374_wdt_info = {
> >  						WDIOF_MAGICCLOSE,
> >  };
> >
> > -static int ds1374_wdt_settimeout(unsigned int timeout)
> > +static int ds1374_wdt_settimeout(struct watchdog_device *wdt,
> > +					unsigned int timeout)
> 
> This fits into one line (new line length limit)
> 
> >  {
> > -	int ret = -ENOIOCTLCMD;
> > -	int cr;
> > +	int ret, cr;
> > +
> > +	wdt->timeout = timeout;
> >
> > -	ret = cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
> > +	cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
> > +	ret = cr;
> >  	if (ret < 0)
> > -		goto out;
> > +		return ret;
> >
> Assignment to ret is pointless. Just return cr if negative.
> 
> >  	/* Disable any existing watchdog/alarm before setting the new one */
> >  	cr &= ~DS1374_REG_CR_WACE;
> >
> >  	ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
> >  	if (ret < 0)
> > -		goto out;
> > +		return ret;
> >
> >  	/* Set new watchdog time */
> > +	timeout = timeout * 4096;
> >  	ret = ds1374_write_rtc(save_client, timeout, DS1374_REG_WDALM0, 3);
> >  	if (ret) {
> >  		pr_info("couldn't set new watchdog time\n");
> > -		goto out;
> > +		return ret;
> >  	}
> >
> >  	/* 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(save_client, DS1374_REG_CR, cr);
> >  	if (ret < 0)
> > -		goto out;
> > +		return ret;
> >
> >  	return 0;
> > -out:
> > -	return ret;
> >  }
> >
> > -
> >  /*
> >   * Reload the watchdog timer.  (ie, pat the watchdog)
> >   */
> > -static void ds1374_wdt_ping(void)
> > +static int ds1374_wdt_ping(struct watchdog_device *wdt)
> >  {
> >  	u32 val;
> > -	int ret = 0;
> >
> > -	ret = ds1374_read_rtc(save_client, &val, DS1374_REG_WDALM0, 3);
> > -	if (ret)
> > -		pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret);
> > +	return ds1374_read_rtc(save_client, &val, DS1374_REG_WDALM0, 3); }
> > +
> > +static int ds1374_wdt_start(struct watchdog_device *wdt) {
> > +	return ds1374_wdt_ping(wdt);
> 
> Unnecessary. Just declare the start function and drop the ping function.
> See Documentation/watchdog/watchdog-kernel-api.rst
> 
> >  }
> >
> > -static void ds1374_wdt_disable(void)
> > +static int ds1374_wdt_stop(struct watchdog_device *wdt)
> >  {
> >  	int cr;
> >
> > @@ -444,162 +456,17 @@ static void ds1374_wdt_disable(void)
> >  	/* Disable watchdog timer */
> >  	cr &= ~DS1374_REG_CR_WACE;
> >
> > -	i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
> > +	return i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
> >  }
> >
> > -/*
> > - * Watchdog device is opened, and watchdog starts running.
> > - */
> > -static int ds1374_wdt_open(struct inode *inode, struct file *file) -{
> > -	struct ds1374 *ds1374 = i2c_get_clientdata(save_client);
> > -
> > -	if (MINOR(inode->i_rdev) == WATCHDOG_MINOR) {
> > -		mutex_lock(&ds1374->mutex);
> > -		if (test_and_set_bit(0, &wdt_is_open)) {
> > -			mutex_unlock(&ds1374->mutex);
> > -			return -EBUSY;
> > -		}
> > -		/*
> > -		 *      Activate
> > -		 */
> > -		wdt_is_open = 1;
> > -		mutex_unlock(&ds1374->mutex);
> > -		return stream_open(inode, file);
> > -	}
> > -	return -ENODEV;
> > -}
> > -
> > -/*
> > - * Close the watchdog device.
> > - */
> > -static int ds1374_wdt_release(struct inode *inode, struct file *file)
> > -{
> > -	if (MINOR(inode->i_rdev) == WATCHDOG_MINOR)
> > -		clear_bit(0, &wdt_is_open);
> > -
> > -	return 0;
> > -}
> > -
> > -/*
> > - * Pat the watchdog whenever device is written to.
> > - */
> > -static ssize_t ds1374_wdt_write(struct file *file, const char __user *data,
> > -				size_t len, loff_t *ppos)
> > -{
> > -	if (len) {
> > -		ds1374_wdt_ping();
> > -		return 1;
> > -	}
> > -	return 0;
> > -}
> > -
> > -static ssize_t ds1374_wdt_read(struct file *file, char __user *data,
> > -				size_t len, loff_t *ppos)
> > -{
> > -	return 0;
> > -}
> > -
> > -/*
> > - * Handle commands from user-space.
> > - */
> > -static long ds1374_wdt_ioctl(struct file *file, 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();
> > -		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(new_margin);
> > -		ds1374_wdt_ping();
> > -		/* 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_disable();
> > -			return 0;
> > -		}
> > -
> > -		if (options & WDIOS_ENABLECARD) {
> > -			pr_info("enable watchdog\n");
> > -			ds1374_wdt_settimeout(wdt_margin);
> > -			ds1374_wdt_ping();
> > -			return 0;
> > -		}
> > -		return -EINVAL;
> > -	}
> > -	return -ENOTTY;
> > -}
> > -
> > -static long ds1374_wdt_unlocked_ioctl(struct file *file, unsigned int cmd,
> > -			unsigned long arg)
> > -{
> > -	int ret;
> > -	struct ds1374 *ds1374 = i2c_get_clientdata(save_client);
> > -
> > -	mutex_lock(&ds1374->mutex);
> > -	ret = ds1374_wdt_ioctl(file, cmd, arg);
> > -	mutex_unlock(&ds1374->mutex);
> > -
> > -	return ret;
> > -}
> > -
> > -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_disable();
> > -	return NOTIFY_DONE;
> > -}
> > -
> > -static const struct file_operations ds1374_wdt_fops = {
> > -	.owner			= THIS_MODULE,
> > -	.read			= ds1374_wdt_read,
> > -	.unlocked_ioctl		= ds1374_wdt_unlocked_ioctl,
> > -	.compat_ioctl		= compat_ptr_ioctl,
> > -	.write			= ds1374_wdt_write,
> > -	.open                   = ds1374_wdt_open,
> > -	.release                = ds1374_wdt_release,
> > -	.llseek			= no_llseek,
> > -};
> > -
> > -static struct miscdevice ds1374_miscdev = {
> > -	.minor          = WATCHDOG_MINOR,
> > -	.name           = "watchdog",
> > -	.fops           = &ds1374_wdt_fops,
> > +static const struct watchdog_ops ds1374_wdt_fops = {
> 
> s/fops/ops/
> 
> Those are not file operations.
> 
> > +	.owner          = THIS_MODULE,
> > +	.start          = ds1374_wdt_start,
> > +	.stop           = ds1374_wdt_stop,
> > +	.ping           = ds1374_wdt_ping,
> > +	.set_timeout    = ds1374_wdt_settimeout,
> >  };
> >
> > -static struct notifier_block ds1374_wdt_notifier = {
> > -	.notifier_call = ds1374_wdt_notify_sys,
> > -};
> >
> >  #endif /*CONFIG_RTC_DRV_DS1374_WDT*/
> >  /*
> > @@ -653,15 +520,25 @@ static int ds1374_probe(struct i2c_client *client,
> >
> >  #ifdef CONFIG_RTC_DRV_DS1374_WDT
> >  	save_client = client;
> > -	ret = misc_register(&ds1374_miscdev);
> > -	if (ret)
> > -		return ret;
> > -	ret = register_reboot_notifier(&ds1374_wdt_notifier);
> > +	ds1374->wdt.info = &ds1374_wdt_info;
> > +	ds1374->wdt.ops = &ds1374_wdt_fops;
> > +	ds1374->wdt.timeout = TIMER_MARGIN_DEFAULT;
> > +	ds1374->wdt.min_timeout = TIMER_MARGIN_MIN;
> > +	ds1374->wdt.max_timeout = TIMER_MARGIN_MAX;
> > +
> > +	watchdog_init_timeout(&ds1374->wdt, timeout, &client->dev);
> > +	watchdog_set_nowayout(&ds1374->wdt, nowayout);
> > +	watchdog_stop_on_reboot(&ds1374->wdt);
> > +	watchdog_stop_on_unregister(&ds1374->wdt);
> > +
> > +	ret = devm_watchdog_register_device(&client->dev, &ds1374->wdt);
> >  	if (ret) {
> > -		misc_deregister(&ds1374_miscdev);
> > +		dev_err(&client->dev, "failed to register DS1374 watchdog
> device\n");
> >  		return ret;
> >  	}
> > -	ds1374_wdt_settimeout(131072);
> > +
> > +	ds1374_wdt_settimeout(&ds1374->wdt, timeout);
> 
> You'll want to do that before registering the watchdog to avoid a
> race condition.
> 
> > +	dev_info(&client->dev, "DS1374 watchdog device enabled\n");
> >  #endif
> >
> >  	return 0;
> > @@ -670,10 +547,10 @@ static int ds1374_probe(struct i2c_client *client,
> >  static int ds1374_remove(struct i2c_client *client)
> >  {
> >  	struct ds1374 *ds1374 = i2c_get_clientdata(client);
> > +
> >  #ifdef CONFIG_RTC_DRV_DS1374_WDT
> > -	misc_deregister(&ds1374_miscdev);
> > -	ds1374_miscdev.parent = NULL;
> > -	unregister_reboot_notifier(&ds1374_wdt_notifier);
> > +	dev_warn(&client->dev, "Unregister DS1374 watchdog device\n");
> 
> Noise.
> 
> > +	watchdog_unregister_device(&ds1374->wdt);
> 
> Drop: The watchdog was registered using devm_watchdog_register_device().
> 

Thanks for your good suggestions. I'll fix them and kernel test robot by v2.

> >  #endif
> >
> >  	if (client->irq > 0) {
> >

Best regards,
Johnson

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-01 10:09 [PATCH] rtc: ds1374: wdt: Use watchdog core for watchdog part Johnson CH Chen (陳昭勳)
2020-07-01 14:05 ` Guenter Roeck
2020-07-03 10:15   ` Johnson CH Chen (陳昭勳)
2020-07-02 12:25 ` kernel test robot
2020-07-02 14:38 ` kernel test robot

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git