[PATCH 1/2 V3] watchdog: Add multiple device support

[PATCH 1/2 V3] watchdog: Add multiple device support

[Tomas Winkler]

From: Alan Cox <alan@linux.intel.com>

We keep the old /dev/watchdog interface file for the first watchdog via
miscdev. This is basically a cut and paste of the relevant interface code
from the rtc driver layer tweaked for watchdog.

Signed-off-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2 (by Wim):
    1. Clean split between core and dev related stuff. 
    2. Use ida instead of the watchdog_assign/release_id code.
    3. Rest it's basically the code that Alan produced with some minor changes
       with the comments that Hans made.
V3: (by Tomas) 
    1. Fix compilation error caused by missing include to watchdog_dev.h
    2. Add documentation changes to be part of this patch
    3. Make init and exit module functions static (fixes sparse error)
    4. Fix minor checkpatch.pl complains

 Documentation/watchdog/watchdog-kernel-api.txt |    6 +
 drivers/watchdog/watchdog_core.c               |   29 +++++-
 drivers/watchdog/watchdog_dev.c                |  131 ++++++++++++++++--------
 drivers/watchdog/watchdog_dev.h                |    2 +
 include/linux/watchdog.h                       |    6 +
 5 files changed, 125 insertions(+), 49 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 227f6cd..2d28950 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -46,10 +46,13 @@ struct watchdog_device {
 	unsigned int min_timeout;
 	unsigned int max_timeout;
 	void *driver_data;
+	int id;
+	struct cdev cdev;
 	unsigned long status;
 };
 
 It contains following fields:
+
 * info: a pointer to a watchdog_info structure. This structure gives some
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
@@ -61,6 +64,9 @@ It contains following fields:
 * driver_data: a pointer to the drivers private data of a watchdog device.
   This data should only be accessed via the watchdog_set_drvadata and
   watchdog_get_drvdata routines.
+* id: set by watchdog_register_device. id 0 is special, it has both cdev with
+  dynamic major and minor 0 (/dev/watchdog0) and as well as the old miscdev /dev/watchdog
+* cdev: cdev for the dynamic major /dev/watchdog<id> device nodes
 * status: this field contains a number of status bits that give extra
   information about the status of the device (Like: is the watchdog timer
   running/active, is the nowayout bit set, is the device opened via
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 14d768b..f6f3828 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -34,9 +34,12 @@
 #include <linux/kernel.h>	/* For printk/panic/... */
 #include <linux/watchdog.h>	/* For watchdog specific items */
 #include <linux/init.h>		/* For __init/__exit/... */
+#include <linux/idr.h>		/* For ida_* macros */
 
 #include "watchdog_dev.h"	/* For watchdog_dev_register/... */
 
+static DEFINE_IDA(watchdog_ida);
+
 /**
  * watchdog_register_device() - register a watchdog device
  * @wdd: watchdog device
@@ -49,7 +52,7 @@
  */
 int watchdog_register_device(struct watchdog_device *wdd)
 {
-	int ret;
+	int ret, id;
 
 	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
 		return -EINVAL;
@@ -74,11 +77,28 @@ int watchdog_register_device(struct watchdog_device *wdd)
 	 * corrupted in a later stage then we expect a kernel panic!
 	 */
 
-	/* We only support 1 watchdog device via the /dev/watchdog interface */
+	id = ida_simple_get(&watchdog_ida, 0, MAX_DOGS, GFP_KERNEL);
+	if (id < 0)
+		return id;
+	wdd->id = id;
+
 	ret = watchdog_dev_register(wdd);
 	if (ret) {
-		pr_err("error registering /dev/watchdog (err=%d)\n", ret);
-		return ret;
+		ida_simple_remove(&watchdog_ida, id);
+		if (!(id == 0 || ret == -EBUSY))
+			return ret;
+
+		/* Retry in case a legacy watchdog module exists */
+		id = ida_simple_get(&watchdog_ida, 1, MAX_DOGS, GFP_KERNEL);
+		if (id < 0)
+			return id;
+		wdd->id = id;
+
+		ret = watchdog_dev_register(wdd);
+		if (ret) {
+			ida_simple_remove(&watchdog_ida, id);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -102,6 +122,7 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
 	ret = watchdog_dev_unregister(wdd);
 	if (ret)
 		pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
+	ida_simple_remove(&watchdog_ida, wdd->id);
 }
 EXPORT_SYMBOL_GPL(watchdog_unregister_device);
 
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 8558da9..4ce3b78 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -41,11 +41,12 @@
 #include <linux/miscdevice.h>	/* For handling misc devices */
 #include <linux/init.h>		/* For __init/__exit/... */
 #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
+#include "watchdog_dev.h"
 
-/* make sure we only register one /dev/watchdog device */
-static unsigned long watchdog_dev_busy;
 /* the watchdog device behind /dev/watchdog */
-static struct watchdog_device *wdd;
+static struct watchdog_device *old_wdd;
+/* the dev_t structure to store the dynamically allocated watchdog devices */
+static dev_t watchdog_devt;
 
 /*
  *	watchdog_ping: ping the watchdog.
@@ -136,6 +137,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
 static ssize_t watchdog_write(struct file *file, const char __user *data,
 						size_t len, loff_t *ppos)
 {
+	struct watchdog_device *wdd = file->private_data;
 	size_t i;
 	char c;
 
@@ -175,6 +177,7 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
 static long watchdog_ioctl(struct file *file, unsigned int cmd,
 							unsigned long arg)
 {
+	struct watchdog_device *wdd = file->private_data;
 	void __user *argp = (void __user *)arg;
 	int __user *p = argp;
 	unsigned int val;
@@ -247,11 +250,11 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 }
 
 /*
- *	watchdog_open: open the /dev/watchdog device.
+ *	watchdog_open: open the /dev/watchdog* devices.
  *	@inode: inode of device
  *	@file: file handle to device
  *
- *	When the /dev/watchdog device gets opened, we start the watchdog.
+ *	When the /dev/watchdog* device gets opened, we start the watchdog.
  *	Watch out: the /dev/watchdog device is single open, so we make sure
  *	it can only be opened once.
  */
@@ -259,6 +262,13 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 static int watchdog_open(struct inode *inode, struct file *file)
 {
 	int err = -EBUSY;
+	struct watchdog_device *wdd;
+
+	/* Get the corresponding watchdog device */
+	if (imajor(inode) == MISC_MAJOR)
+		wdd = old_wdd;
+	else
+		wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);
 
 	/* the watchdog is single open! */
 	if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
@@ -275,6 +285,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
 	if (err < 0)
 		goto out_mod;
 
+	file->private_data = wdd;
+
 	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
 	return nonseekable_open(inode, file);
 
@@ -286,9 +298,9 @@ out:
 }
 
 /*
- *      watchdog_release: release the /dev/watchdog device.
- *      @inode: inode of device
- *      @file: file handle to device
+ *	watchdog_release: release the watchdog device.
+ *	@inode: inode of device
+ *	@file: file handle to device
  *
  *	This is the code for when /dev/watchdog gets closed. We will only
  *	stop the watchdog when we have received the magic char (and nowayout
@@ -297,6 +309,7 @@ out:
 
 static int watchdog_release(struct inode *inode, struct file *file)
 {
+	struct watchdog_device *wdd = file->private_data;
 	int err = -EBUSY;
 
 	/*
@@ -338,62 +351,90 @@ static struct miscdevice watchdog_miscdev = {
 };
 
 /*
- *	watchdog_dev_register:
+ *	watchdog_dev_register: register a watchdog device
  *	@watchdog: watchdog device
  *
- *	Register a watchdog device as /dev/watchdog. /dev/watchdog
- *	is actually a miscdevice and thus we set it up like that.
+ *	Register a watchdog device including handling the legacy
+ *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and
+ *	thus we set it up like that.
  */
 
 int watchdog_dev_register(struct watchdog_device *watchdog)
 {
-	int err;
-
-	/* Only one device can register for /dev/watchdog */
-	if (test_and_set_bit(0, &watchdog_dev_busy)) {
-		pr_err("only one watchdog can use /dev/watchdog\n");
-		return -EBUSY;
+	int err, devno;
+
+	if (watchdog->id == 0) {
+		err = misc_register(&watchdog_miscdev);
+		if (err != 0) {
+			pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
+				watchdog->info->identity, WATCHDOG_MINOR, err);
+			if (err == -EBUSY)
+				pr_err("%s: a legacy watchdog module is probably present.\n",
+					watchdog->info->identity);
+			return err;
+		}
+		old_wdd = watchdog;
 	}
 
-	wdd = watchdog;
-
-	err = misc_register(&watchdog_miscdev);
-	if (err != 0) {
-		pr_err("%s: cannot register miscdev on minor=%d (err=%d)\n",
-		       watchdog->info->identity, WATCHDOG_MINOR, err);
-		goto out;
+	/* Fill in the data structures */
+	devno = MKDEV(MAJOR(watchdog_devt), watchdog->id);
+	cdev_init(&watchdog->cdev, &watchdog_fops);
+	watchdog->cdev.owner = watchdog->ops->owner;
+
+	/* Add the device */
+	err  = cdev_add(&watchdog->cdev, devno, 1);
+	if (err) {
+		pr_err("watchdog%d unable to add device %d:%d\n",
+			watchdog->id,  MAJOR(watchdog_devt), watchdog->id);
+		if (watchdog->id == 0) {
+			misc_deregister(&watchdog_miscdev);
+			old_wdd = NULL;
+		}
 	}
-
-	return 0;
-
-out:
-	wdd = NULL;
-	clear_bit(0, &watchdog_dev_busy);
 	return err;
 }
 
 /*
- *	watchdog_dev_unregister:
+ *	watchdog_dev_unregister: unregister a watchdog device
  *	@watchdog: watchdog device
  *
- *	Deregister the /dev/watchdog device.
+ *	Unregister the watchdog and if needed the legacy /dev/watchdog device.
  */
 
 int watchdog_dev_unregister(struct watchdog_device *watchdog)
 {
-	/* Check that a watchdog device was registered in the past */
-	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
-		return -ENODEV;
-
-	/* We can only unregister the watchdog device that was registered */
-	if (watchdog != wdd) {
-		pr_err("%s: watchdog was not registered as /dev/watchdog\n",
-		       watchdog->info->identity);
-		return -ENODEV;
+	cdev_del(&watchdog->cdev);
+	if (watchdog->id == 0) {
+		misc_deregister(&watchdog_miscdev);
+		old_wdd = NULL;
 	}
-
-	misc_deregister(&watchdog_miscdev);
-	wdd = NULL;
-	clear_bit(0, &watchdog_dev_busy);
 	return 0;
 }
+
+/*
+ *	watchdog_init: module init call
+ *
+ *	Allocate a range of chardev nodes to use for watchdog devices
+ */
+
+static int __init watchdog_init(void)
+{
+	int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
+	if (err < 0)
+		pr_err("watchdog: unable to allocate char dev region\n");
+	return err;
+}
+
+/*
+ *	watchdog_exit: module exit
+ *
+ *	Release the range of chardev nodes used for watchdog devices
+ */
+
+static void __exit watchdog_exit(void)
+{
+	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
+}
+
+module_init(watchdog_init);
+module_exit(watchdog_exit);
diff --git a/drivers/watchdog/watchdog_dev.h b/drivers/watchdog/watchdog_dev.h
index bc7612b..0eb8f6f 100644
--- a/drivers/watchdog/watchdog_dev.h
+++ b/drivers/watchdog/watchdog_dev.h
@@ -26,6 +26,8 @@
  *	This material is provided "AS-IS" and at no charge.
  */
 
+#define MAX_DOGS	32	/* Maximum number of watchdog devices */
+
 /*
  *	Functions/procedures to be called by the core
  */
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index ac40716..1397cbc 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -54,6 +54,8 @@ struct watchdog_info {
 #ifdef __KERNEL__
 
 #include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/cdev.h>
 
 struct watchdog_ops;
 struct watchdog_device;
@@ -96,6 +98,8 @@ struct watchdog_ops {
  * @min_timeout:The watchdog devices minimum timeout value.
  * @max_timeout:The watchdog devices maximum timeout value.
  * @driver-data:Pointer to the drivers private data.
+ * @id:		The watchdog's ID.
+ * @cdev:	The watchdog's Character device.
  * @status:	Field that contains the devices internal status bits.
  *
  * The watchdog_device structure contains all information about a
@@ -112,6 +116,8 @@ struct watchdog_device {
 	unsigned int min_timeout;
 	unsigned int max_timeout;
 	void *driver_data;
+	int id;
+	struct cdev cdev;
 	unsigned long status;
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */
-- 
1.7.4.4

[PATCH 2/2 V3] watchdog: create all the proper device files

[Tomas Winkler]

From: Alan Cox <alan@linux.intel.com>

Signed-off-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
V2,V3: Rebase

 drivers/watchdog/watchdog_dev.c |   38 ++++++++++++++++++++++++++++++++------
 include/linux/watchdog.h        |    4 ++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 4ce3b78..0b6a774 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -47,6 +47,8 @@
 static struct watchdog_device *old_wdd;
 /* the dev_t structure to store the dynamically allocated watchdog devices */
 static dev_t watchdog_devt;
+/* the class for the dynamically allocated watchdog devices */
+static struct class *watchdog_class;
 
 /*
  *	watchdog_ping: ping the watchdog.
@@ -386,10 +388,22 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
 	if (err) {
 		pr_err("watchdog%d unable to add device %d:%d\n",
 			watchdog->id,  MAJOR(watchdog_devt), watchdog->id);
-		if (watchdog->id == 0) {
-			misc_deregister(&watchdog_miscdev);
-			old_wdd = NULL;
-		}
+		goto error;
+	}
+	watchdog->dev = device_create(watchdog_class, watchdog->busdev, devno,
+					NULL, "watchdog%d", watchdog->id);
+	if (IS_ERR(watchdog->dev)) {
+		cdev_del(&watchdog->cdev);
+		err = PTR_ERR(watchdog->dev);
+		goto error;
+	}
+
+	return 0;
+
+error:
+	if (watchdog->id == 0) {
+		misc_deregister(&watchdog_miscdev);
+		old_wdd = NULL;
 	}
 	return err;
 }
@@ -404,6 +418,10 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
 int watchdog_dev_unregister(struct watchdog_device *watchdog)
 {
 	cdev_del(&watchdog->cdev);
+	device_destroy(watchdog_class,
+		       MKDEV(MAJOR(watchdog_devt), watchdog->id));
+	watchdog->dev = NULL;
+
 	if (watchdog->id == 0) {
 		misc_deregister(&watchdog_miscdev);
 		old_wdd = NULL;
@@ -420,9 +438,16 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog)
 static int __init watchdog_init(void)
 {
 	int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
-	if (err < 0)
+	if (err < 0) {
 		pr_err("watchdog: unable to allocate char dev region\n");
-	return err;
+		return err;
+	}
+	watchdog_class = class_create(THIS_MODULE, "watchdog");
+	if (IS_ERR(watchdog_class)) {
+		unregister_chrdev_region(watchdog_devt, MAX_DOGS);
+		return PTR_ERR(watchdog_class);
+	}
+	return 0;
 }
 
 /*
@@ -434,6 +459,7 @@ static int __init watchdog_init(void)
 static void __exit watchdog_exit(void)
 {
 	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
+	class_destroy(watchdog_class);
 }
 
 module_init(watchdog_init);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 1397cbc..522aa61 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -100,6 +100,8 @@ struct watchdog_ops {
  * @driver-data:Pointer to the drivers private data.
  * @id:		The watchdog's ID.
  * @cdev:	The watchdog's Character device.
+ * @dev:	The device for our node
+ * @busdev:	Parent bus device
  * @status:	Field that contains the devices internal status bits.
  *
  * The watchdog_device structure contains all information about a
@@ -118,6 +120,8 @@ struct watchdog_device {
 	void *driver_data;
 	int id;
 	struct cdev cdev;
+	struct device *dev;
+	struct device *busdev;
 	unsigned long status;
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */
-- 
1.7.4.4

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Tomas,

> From: Alan Cox <alan@linux.intel.com>
> 
> We keep the old /dev/watchdog interface file for the first watchdog via
> miscdev. This is basically a cut and paste of the relevant interface code
> from the rtc driver layer tweaked for watchdog.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
> V2 (by Wim):
>     1. Clean split between core and dev related stuff. 
>     2. Use ida instead of the watchdog_assign/release_id code.
>     3. Rest it's basically the code that Alan produced with some minor changes
>        with the comments that Hans made.
> V3: (by Tomas) 
>     1. Fix compilation error caused by missing include to watchdog_dev.h

Is allready in linux-watchdog-next (see commit 1761f6851b23e1cd61705c4c053decabb14f9092)

>     2. Add documentation changes to be part of this patch
>     3. Make init and exit module functions static (fixes sparse error)
>     4. Fix minor checkpatch.pl complains

Kind regards,
Wim.

RE: [PATCH 1/2 V3] watchdog: Add multiple device support

[Winkler, Tomas]

> changes
> >        with the comments that Hans made.
> > V3: (by Tomas)
> >     1. Fix compilation error caused by missing include to
> > watchdog_dev.h
> 
> Is allready in linux-watchdog-next (see commit
> 1761f6851b23e1cd61705c4c053decabb14f9092)

Hmm  didn't know that -next exists till now... I'll have a look.
Thanks
Tomas 


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

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Tomas,

>  	ret = watchdog_dev_register(wdd);
>  	if (ret) {
> -		pr_err("error registering /dev/watchdog (err=%d)\n", ret);
> -		return ret;
> +		ida_simple_remove(&watchdog_ida, id);
> +		if (!(id == 0 || ret == -EBUSY))
> +			return ret;
> +
> +		/* Retry in case a legacy watchdog module exists */

Nope, this should stay
if (!(id == 0 && ret == -EBUSY))

because we will only retry, if we have the possibility that another legacy watchdog is allready there:
i.e. when the id == 0 and when we have a -EBUSY as a return code.A

Kind regards,
Wim.

RE: [PATCH 1/2 V3] watchdog: Add multiple device support

[Winkler, Tomas]

> Hi Tomas,
> 
> >  	ret = watchdog_dev_register(wdd);
> >  	if (ret) {
> > -		pr_err("error registering /dev/watchdog (err=%d)\n", ret);
> > -		return ret;
> > +		ida_simple_remove(&watchdog_ida, id);
> > +		if (!(id == 0 || ret == -EBUSY))
> > +			return ret;
> > +
> > +		/* Retry in case a legacy watchdog module exists */
> 
> Nope, this should stay
> if (!(id == 0 && ret == -EBUSY))
> 
> because we will only retry, if we have the possibility that another legacy
> watchdog is allready there:
> i.e. when the id == 0 and when we have a -EBUSY as a return code.A

You are right... I've wrote the statement (id == 0 || ret == -EBUSY)  and then saw the that readability 
comment  and forgot to change the II into && as well.
Thanks
Tomas


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

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Tomas Winkler]

On Mon, May 14, 2012 at 5:00 PM, Winkler, Tomas <tomas.winkler@intel.com> wrote:
>> changes
>> >        with the comments that Hans made.
>> > V3: (by Tomas)
>> >     1. Fix compilation error caused by missing include to
>> > watchdog_dev.h
>>
>> Is allready in linux-watchdog-next (see commit
>> 1761f6851b23e1cd61705c4c053decabb14f9092)
>
> Hmm  didn't know that -next exists till now... I'll have a look.
> Thanks
> Tomas

I see you've add my changes into the patch and  rebased that tree,
usually  this is not preferable strategy for public git tree as it
creates conflicts.
Should we expect the -next tree to be rebased from time to time?

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

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Tomas,

> I see you've add my changes into the patch and  rebased that tree,
> usually  this is not preferable strategy for public git tree as it
> creates conflicts.
> Should we expect the -next tree to be rebased from time to time?

I try to avoid rebasing, but it's sometimes better to change new patches
(although sometimes it's better not to). So Yes a rebase is possible in
linux-watchdog-next. (Which is according to the rules of linux-next).

Kind regards,
Wim.

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Tomas Winkler]

On Thu, May 17, 2012 at 10:19 AM, Wim Van Sebroeck <wim@iguana.be> wrote:
>
> Hi Tomas,
>
> > I see you've add my changes into the patch and  rebased that tree,
> > usually  this is not preferable strategy for public git tree as it
> > creates conflicts.
> > Should we expect the -next tree to be rebased from time to time?
>
> I try to avoid rebasing, but it's sometimes better to change new patches
> (although sometimes it's better not to). So Yes a rebase is possible in
> linux-watchdog-next. (Which is according to the rules of linux-next).
>
Thanks for clarification.
BTW the patch is gone again from the tree.

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

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Tomas,

> Thanks for clarification.
> BTW the patch is gone again from the tree.

Patches are back but I added some small fixes:
* watchdog_dev.h should have been watchdog_core.h
* the file should have been using extern for the function prototypes.
* introduce subsys_initcall instead of module_init and move that also to the core.
* make subsys_initcall and module_exit static
* moved device create code into watchdog_core where it belongs
* changed busdev to parent
* integrated Hans's documentation into the respective patches.

Please have a look and test.

Kind regards,
Wim.

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Hans de Goede]

Hi,

On 05/21/2012 10:21 PM, Wim Van Sebroeck wrote:
> Hi Tomas,
>
>> Thanks for clarification.
>> BTW the patch is gone again from the tree.
>
> Patches are back but I added some small fixes:
> * watchdog_dev.h should have been watchdog_core.h
> * the file should have been using extern for the function prototypes.
> * introduce subsys_initcall instead of module_init and move that also to the core.
> * make subsys_initcall and module_exit static
> * moved device create code into watchdog_core where it belongs
> * changed busdev to parent
> * integrated Hans's documentation into the respective patches.
>
> Please have a look and test.

Thanks for all the work on merging this, I've re-reviewed the set and
tested it with my convert sch56xx watchdog to the wdog-core patchset.

Everything looks good and works as advertised :)

I've rebased my sch56xx watchdog patchset as my patch titled:
"watchdog_dev: Add support for dynamically allocated watchdog_device structs"
needed rebasing, I'll re-send it after this mail.

Regards,

Hans

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Tomas Winkler]

On Mon, May 21, 2012 at 11:21 PM, Wim Van Sebroeck <wim@iguana.be> wrote:
> Hi Tomas,
>
>> Thanks for clarification.
>> BTW the patch is gone again from the tree.
>
> Patches are back but I added some small fixes:
> * watchdog_dev.h should have been watchdog_core.h
> * the file should have been using extern for the function prototypes.
> * introduce subsys_initcall instead of module_init and move that also to the core.
> * make subsys_initcall and module_exit static
> * moved device create code into watchdog_core where it belongs
> * changed busdev to parent
> * integrated Hans's documentation into the respective patches.
>
> Please have a look and test.
>

I think it would be better if we stick the the development process and
and the patches are send to the mailing list for review
Anyhow I'm not able to fetch the repository today, I'm not sure yet on
what side is the problem

'linux-watchdog]$ git fetch
fatal: The remote end hung up unexpectedly
'

Thanks
Tomw

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Tomas,

> I think it would be better if we stick the the development process and
> and the patches are send to the mailing list for review

Didn't had time left yesterday to do that still.

> Anyhow I'm not able to fetch the repository today, I'm not sure yet on
> what side is the problem
>
> 'linux-watchdog]$ git fetch
> fatal: The remote end hung up unexpectedly
> '

Git daemon crashed for some reason. It has been restarted.

Kind regards,
Wim.

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Hans,

> >>Thanks for clarification.
> >>BTW the patch is gone again from the tree.
> >
> >Patches are back but I added some small fixes:
> >* watchdog_dev.h should have been watchdog_core.h
> >* the file should have been using extern for the function prototypes.
> >* introduce subsys_initcall instead of module_init and move that also to 
> >the core.
> >* make subsys_initcall and module_exit static
> >* moved device create code into watchdog_core where it belongs
> >* changed busdev to parent
> >* integrated Hans's documentation into the respective patches.
> >
> >Please have a look and test.
> 
> Thanks for all the work on merging this, I've re-reviewed the set and
> tested it with my convert sch56xx watchdog to the wdog-core patchset.
> 
> Everything looks good and works as advertised :)
> 
> I've rebased my sch56xx watchdog patchset as my patch titled:
> "watchdog_dev: Add support for dynamically allocated watchdog_device 
> structs"
> needed rebasing, I'll re-send it after this mail.

I will split your "watchdog_dev: Add support for dynamically allocated
watchdog_device structs" patch into 3 patches:
1) rewriting of wrappers (and I will add some extra code to it for
the get_timeleft ioctl).
2) locking
3) ref+unref+unregistered.

Hope to do that tonight.

Kind regards,
Wim.

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Hans de Goede]

Hi,

On 05/22/2012 06:41 PM, Wim Van Sebroeck wrote:
> Hi Hans,
>
>>>> Thanks for clarification.
>>>> BTW the patch is gone again from the tree.
>>>
>>> Patches are back but I added some small fixes:
>>> * watchdog_dev.h should have been watchdog_core.h
>>> * the file should have been using extern for the function prototypes.
>>> * introduce subsys_initcall instead of module_init and move that also to
>>> the core.
>>> * make subsys_initcall and module_exit static
>>> * moved device create code into watchdog_core where it belongs
>>> * changed busdev to parent
>>> * integrated Hans's documentation into the respective patches.
>>>
>>> Please have a look and test.
>>
>> Thanks for all the work on merging this, I've re-reviewed the set and
>> tested it with my convert sch56xx watchdog to the wdog-core patchset.
>>
>> Everything looks good and works as advertised :)
>>
>> I've rebased my sch56xx watchdog patchset as my patch titled:
>> "watchdog_dev: Add support for dynamically allocated watchdog_device
>> structs"
>> needed rebasing, I'll re-send it after this mail.
>
> I will split your "watchdog_dev: Add support for dynamically allocated
> watchdog_device structs" patch into 3 patches:
> 1) rewriting of wrappers (and I will add some extra code to it for
> the get_timeleft ioctl).
> 2) locking
> 3) ref+unref+unregistered.

Sounds good! Assuming you manage to finish this tonight, I'll review
and test first thing tomorrow morning.

Regards,

Hans

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Hans,

> >I will split your "watchdog_dev: Add support for dynamically allocated
> >watchdog_device structs" patch into 3 patches:
> >1) rewriting of wrappers (and I will add some extra code to it for
> >the get_timeleft ioctl).
> >2) locking
> >3) ref+unref+unregistered.
> 
> Sounds good! Assuming you manage to finish this tonight, I'll review
> and test first thing tomorrow morning.

They are there.

Kind regards,
Wim.

Re: [PATCH 1/2 V3] watchdog: Add multiple device support

[Hans de Goede]

Hi,

On 05/22/2012 11:18 PM, Wim Van Sebroeck wrote:
> Hi Hans,
>
>>> I will split your "watchdog_dev: Add support for dynamically allocated
>>> watchdog_device structs" patch into 3 patches:
>>> 1) rewriting of wrappers (and I will add some extra code to it for
>>> the get_timeleft ioctl).
>>> 2) locking
>>> 3) ref+unref+unregistered.
>>
>> Sounds good! Assuming you manage to finish this tonight, I'll review
>> and test first thing tomorrow morning.
>
> They are there.

Thanks!

I've reviewed the entire set, it looks good. I like the 3-way split you did!

I've ofcourse also tested it and it works as advertised.

Regards,

Hans