[MPCore Watchdog]: Convert from misc_dev to dynamic device node.

The current MPCore watchdog driver uses a misc_dev device node.
This patch changes it to use dynamically allocated device numbers.

---
 drivers/watchdog/mpcore_wdt.c |   48 +++++++++++++++++++++++++++++------------
 1 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 2b4af22..f6afc20 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c

@@ -32,6 +32,8 @@
 #include <linux/uaccess.h>
 #include <linux/slab.h>
 #include <linux/io.h>
+#include <linux/cdev.h>
+#include <linux/device.h>

 #include <asm/smp_twd.h>

@@ -42,6 +44,9 @@ struct mpcore_wdt {
 	int		irq;
 	unsigned int	perturb;
 	char		expect_close;
+        struct cdev     cdev;
+        struct class    *class;
+        dev_t           number;
 };

 static struct platform_device *mpcore_wdt_dev;

@@ -318,12 +323,6 @@ static const struct file_operations mpcore_wdt_fops = {
 	.release	= mpcore_wdt_release,
 };

-static struct miscdevice mpcore_wdt_miscdev = {
-	.minor		= WATCHDOG_MINOR,
-	.name		= "watchdog",
-	.fops		= &mpcore_wdt_fops,
-};
-
 static int __devinit mpcore_wdt_probe(struct platform_device *dev)
 {
 	struct mpcore_wdt *wdt;

@@ -358,14 +357,15 @@ static int __devinit mpcore_wdt_probe(struct

platform_device *dev)
 		goto err_free;
 	}

-	mpcore_wdt_miscdev.parent = &dev->dev;
-	ret = misc_register(&mpcore_wdt_miscdev);
-	if (ret) {
+ 	ret = alloc_chrdev_region(&wdt->number, 0, 1, "mpcore_wdt");
+	if (ret < 0) {
 		dev_printk(KERN_ERR, wdt->dev,
-			"cannot register miscdev on minor=%d (err=%d)\n",
-							WATCHDOG_MINOR, ret);
+			"cannot register with dynamic device number (err=%d)\n", -ret);
 		goto err_misc;
 	}
+	dev_printk(KERN_INFO, wdt->dev, "using device number %d, %d",
MAJOR(wdt->number), MINOR(wdt->number));
+
+        cdev_init(&wdt->cdev, &mpcore_wdt_fops);

 	ret = request_irq(wdt->irq, mpcore_wdt_fire, IRQF_DISABLED,
 							"mpcore_wdt", wdt);

@@ -379,10 +379,24 @@ static int __devinit mpcore_wdt_probe(struct

platform_device *dev)
 	platform_set_drvdata(dev, wdt);
 	mpcore_wdt_dev = dev;

-	return 0;
+        ret = cdev_add(&wdt->cdev, wdt->number, 1);
+	if (ret < 0) {
+		dev_printk(KERN_ERR, wdt->dev, "failed to add character device\n");
+		goto err_cdev_add;
+	}

+        /* create /dev/watchdog
+         * we use udev to make the file
+         */
+        wdt->class = class_create(THIS_MODULE,"watchdog");
+        (void) device_create(wdt->class, wdt->dev,
wdt->number,NULL,"watchdog");
+
+        return 0;
+
+err_cdev_add:
+	free_irq(wdt->irq, wdt);
 err_irq:
-	misc_deregister(&mpcore_wdt_miscdev);
+        unregister_chrdev_region (wdt->number, 1);
 err_misc:
 	iounmap(wdt->base);
 err_free:

@@ -395,9 +409,15 @@ static int __devexit mpcore_wdt_remove(struct

platform_device *dev)
 {
 	struct mpcore_wdt *wdt = platform_get_drvdata(dev);

+        device_destroy(wdt->class,wdt->number);
+        class_unregister(wdt->class);
+        class_destroy(wdt->class);
+
+        cdev_del(&wdt->cdev);
+
 	platform_set_drvdata(dev, NULL);

-	misc_deregister(&mpcore_wdt_miscdev);
+        unregister_chrdev_region (wdt->number, 1);

 	mpcore_wdt_dev = NULL;

-- 
1.7.0.4

Hi Peter,

On Tue, Jun 14, 2011 at 04:29:26PM -0700, Peter Fordham wrote:

The current MPCore watchdog driver uses a misc_dev device node.
This patch changes it to use dynamically allocated device numbers.

I'm not sure that this is the correct thing to do.  All other watchdog devices 
use a miscdevice with a major:minor of 10:130, is there a specific reason 
that this node needs to be dynamic?

Also few other comments inline.

Jamie

It looks like tabs have been converted to spaces here.

The return value of class_create() and device_create() should be checked here 
and handled appropriately.  I believe the sysfs classes are pretty much 
deprecated now in preference of a bus too.

class_destroy() calls calls_unregister() internally so you wouldn't need to 
call this.

+        cdev_del(&wdt->cdev);
+
 	platform_set_drvdata(dev, NULL);

-	misc_deregister(&mpcore_wdt_miscdev);
+        unregister_chrdev_region (wdt->number, 1);

 	mpcore_wdt_dev = NULL;

-- 
1.7.0.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Jun 14, 2011 at 04:29:26PM -0700, Peter Fordham wrote:

quoted

The current MPCore watchdog driver uses a misc_dev device node.
This patch changes it to use dynamically allocated device numbers.

I'm not sure that this is the correct thing to do. ?All other watchdog devices
use a miscdevice with a major:minor of 10:130, is there a specific reason
that this node needs to be dynamic?

I was under the impressions that dynamic device nodes were the way of the
future. Is that not the case?

I'll add the relevant checks in other places as per your suggestions.

I believe the sysfs classes are pretty much
deprecated now in preference of a bus too.

Can you give me some more info here? I thought the sysfs stuff was the
new right way of doing stuff. The class stuff allows udev to automatically
create the right device node.

thanks,


-Pete

On Tue, Jun 14, 2011 at 05:19:06PM -0700, Peter Fordham wrote:

quoted

On Tue, Jun 14, 2011 at 04:29:26PM -0700, Peter Fordham wrote:

quoted

The current MPCore watchdog driver uses a misc_dev device node.
This patch changes it to use dynamically allocated device numbers.

I'm not sure that this is the correct thing to do. ?All other watchdog devices
use a miscdevice with a major:minor of 10:130, is there a specific reason
that this node needs to be dynamic?

I was under the impressions that dynamic device nodes were the way of the
future. Is that not the case?

Well they are for new devices/subsystems but watchdog has an established 
major:minor pair that all other devices use so you don't really have to 
worry about a namespace clash.

I'll add the relevant checks in other places as per your suggestions.

I've added Wim (watchdog driver maintainer), but I would think that if 
this change is worth doing then it should be done for all drivers.

quoted

I believe the sysfs classes are pretty much
deprecated now in preference of a bus too.

Can you give me some more info here? I thought the sysfs stuff was the
new right way of doing stuff. The class stuff allows udev to automatically
create the right device node.

So here's one that I'm aware of https://lkml.org/lkml/2011/3/25/502.  So 
there's nothing wrong with using sysfs and a bus, but certainly a bus is 
preferred over a class.

Jamie

I've added Wim (watchdog driver maintainer), but I would think that if
this change is worth doing then it should be done for all drivers.

Agreed, but today this may be more work than I'm strictly interested in
doing. If we do decide it's the right thing then I'll look into converting the
other drivers later.

quoted

quoted

I believe the sysfs classes are pretty much
deprecated now in preference of a bus too.

Can you give me some more info here? I thought the sysfs stuff was the
new right way of doing stuff. The class stuff allows udev to automatically
create the right device node.

So here's one that I'm aware of https://lkml.org/lkml/2011/3/25/502. ?So
there's nothing wrong with using sysfs and a bus, but certainly a bus is
preferred over a class.

I still don't really get this. If I don't use the class code my /dev node gets
the name /dev/mpcore_wdt instaed of /dev/watchdog. Is the right thing
to do to use a udev rule or is there a way to tell udev via sysfs to make
/dev/watchdog instead of /dev/mpcore_wdt?

Pete

On Wed, Jun 15, 2011 at 11:57:12AM -0700, Peter Fordham wrote:

quoted

I've added Wim (watchdog driver maintainer), but I would think that if
this change is worth doing then it should be done for all drivers.

Agreed, but today this may be more work than I'm strictly interested in
doing. If we do decide it's the right thing then I'll look into converting the
other drivers later.

So I'm not fully aware of what particular problem this is solving, but 
if it is needed for this driver then it should really be done for all of 
them.  That's probably one for Wim to answer though.  Wim posted a 
series for a generic watchdog framework a while back[1] but this still 
made use of a misc device.

quoted

quoted

quoted

I believe the sysfs classes are pretty much
deprecated now in preference of a bus too.

Can you give me some more info here? I thought the sysfs stuff was the
new right way of doing stuff. The class stuff allows udev to automatically
create the right device node.

So here's one that I'm aware of https://lkml.org/lkml/2011/3/25/502. ?So
there's nothing wrong with using sysfs and a bus, but certainly a bus is
preferred over a class.

I still don't really get this. If I don't use the class code my /dev node gets
the name /dev/mpcore_wdt instaed of /dev/watchdog. Is the right thing
to do to use a udev rule or is there a way to tell udev via sysfs to make
/dev/watchdog instead of /dev/mpcore_wdt?

If you use a bus then the device creation is slightly more involved than 
with a class - create the bus, allocate the device and assign the 
bus_type then set the name with dev_set_name().  That should be enough 
for udev to work out the name of the device.

Jamie

1. http://www.spinics.net/lists/linux-watchdog/msg00082.html

The framework looks great. I hope it gets in soon.

If you use a bus then the device creation is slightly more involved than
with a class - create the bus, allocate the device and assign the
bus_type then set the name with dev_set_name(). ?That should be enough
for udev to work out the name of the device.

It's a platform device so it's already on the platform bus under:

/sys/devices/platform/mpcore_wdt

Do I need to create another bus? The platform driver name is used in driver
matching so it doesn't seem like the right thing to do to change it to watchdog.

-Pete

On Wednesday 15 June 2011 21:09:45 Jamie Iles wrote:

On Wed, Jun 15, 2011 at 11:57:12AM -0700, Peter Fordham wrote:

quoted

quoted

I've added Wim (watchdog driver maintainer), but I would think that if
this change is worth doing then it should be done for all drivers.

Agreed, but today this may be more work than I'm strictly interested in
doing. If we do decide it's the right thing then I'll look into converting the
other drivers later.

So I'm not fully aware of what particular problem this is solving, but 
if it is needed for this driver then it should really be done for all of 
them.  That's probably one for Wim to answer though.  Wim posted a 
series for a generic watchdog framework a while back[1] but this still 
made use of a misc device.

Let's first wait until the generic watchdog framework gets merged and
the drivers are converted. After that is in, we can discuss further changes.

I highly doubt that making an incompatible API change benefits anyone here,
but it certainly shouldn't be done for a single driver that is separate from
the framework.

	Arnd

It's a platform device so it's already on the platform bus under:

/sys/devices/platform/mpcore_wdt

sorry, that should have been: /sys/bus/platform/devices/mpcore_wdt.

On 15 June 2011 12:36, Arnd Bergmann [off-list ref] wrote:

Let's first wait until the generic watchdog framework gets merged and
the drivers are converted. After that is in, we can discuss further changes.

That seems logical and reasonable. Are we saying no changes to watchdog
drivers in general until this is done? because I have other patches
for this driver
that among other things actually make it work properly. As it stands today
the timeout calculations are broken which results in random reboots.

The framework patch was submitted 3.5 months ago and isn't in yet. Is it held up
on something? I don't see any negative comments.

I highly doubt that making an incompatible API change benefits anyone here,
but it certainly shouldn't be done for a single driver that is separate from
the framework.

Not to be a pedant but, this doesn't change the API at all.

-Pete

On Wednesday 15 June 2011 21:49:43 Peter Fordham wrote:

On 15 June 2011 12:36, Arnd Bergmann [off-list ref] wrote:

quoted

Let's first wait until the generic watchdog framework gets merged and
the drivers are converted. After that is in, we can discuss further changes.

That seems logical and reasonable. Are we saying no changes to watchdog
drivers in general until this is done? because I have other patches
for this driver
that among other things actually make it work properly. As it stands today
the timeout calculations are broken which results in random reboots.

I mean only user-visible changes.
 

The framework patch was submitted 3.5 months ago and isn't in yet. Is it held up
on something? I don't see any negative comments.

I think Wim has been working on this on and off for years. I'm trying to
encourage him to just put it into linux-next now and merge the stuff for
the 3.1 merge window. I think having over 100 drivers implementing the
same interface with trivial differences is no fun any more.

quoted

I highly doubt that making an incompatible API change benefits anyone here,
but it certainly shouldn't be done for a single driver that is separate from
the framework.

Not to be a pedant but, this doesn't change the API at all.

It does change the default name of the device node, and the major/minor
number, which are user-visible. It's quite possible that there are systems
relying on static device nodes with this driver, and I'm rather sure that
that are systems around relying on static device nodes with other
watchdogs.

	Arnd

Hi All,

On Tue, Jun 14, 2011 at 05:19:06PM -0700, Peter Fordham wrote:

quoted

quoted

On Tue, Jun 14, 2011 at 04:29:26PM -0700, Peter Fordham wrote:

quoted

The current MPCore watchdog driver uses a misc_dev device node.
This patch changes it to use dynamically allocated device numbers.

I'm not sure that this is the correct thing to do. ?All other watchdog devices
use a miscdevice with a major:minor of 10:130, is there a specific reason
that this node needs to be dynamic?

I was under the impressions that dynamic device nodes were the way of the
future. Is that not the case?

Well they are for new devices/subsystems but watchdog has an established 
major:minor pair that all other devices use so you don't really have to 
worry about a namespace clash.

quoted

I'll add the relevant checks in other places as per your suggestions.

I've added Wim (watchdog driver maintainer), but I would think that if 
this change is worth doing then it should be done for all drivers.

quoted

quoted

I believe the sysfs classes are pretty much
deprecated now in preference of a bus too.

Can you give me some more info here? I thought the sysfs stuff was the
new right way of doing stuff. The class stuff allows udev to automatically
create the right device node.

So here's one that I'm aware of https://lkml.org/lkml/2011/3/25/502.  So 
there's nothing wrong with using sysfs and a bus, but certainly a bus is 
preferred over a class.

My opinion: we stick with the /dev/watchdog interface, we don't go for dynamic
device nodes but go for a sysfs interface. And this for all drivers and via the
new watchdog API. (updated version will be sent out today). Makes no sense
to do this for every driver.

Kind regards,
Wim.

Hi All,

quoted

I've added Wim (watchdog driver maintainer), but I would think that if
this change is worth doing then it should be done for all drivers.

Agreed, but today this may be more work than I'm strictly interested in
doing. If we do decide it's the right thing then I'll look into converting the
other drivers later.

No, we need to do it by first converting all drivers to the new API and then a
change of the API will take care of it.

Kind regards,
Wim.

Hi Arnd,

On Wednesday 15 June 2011 21:09:45 Jamie Iles wrote:

quoted

On Wed, Jun 15, 2011 at 11:57:12AM -0700, Peter Fordham wrote:

quoted

quoted

I've added Wim (watchdog driver maintainer), but I would think that if
this change is worth doing then it should be done for all drivers.

Agreed, but today this may be more work than I'm strictly interested in
doing. If we do decide it's the right thing then I'll look into converting the
other drivers later.

So I'm not fully aware of what particular problem this is solving, but 
if it is needed for this driver then it should really be done for all of 
them.  That's probably one for Wim to answer though.  Wim posted a 
series for a generic watchdog framework a while back[1] but this still 
made use of a misc device.

Let's first wait until the generic watchdog framework gets merged and
the drivers are converted. After that is in, we can discuss further changes.

I highly doubt that making an incompatible API change benefits anyone here,
but it certainly shouldn't be done for a single driver that is separate from
the framework.

Agree for the full 100%.

Thanks,
Wim.