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

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

[Peter Fordham]

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

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

[Jamie Iles]

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

> ---
>  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;

It looks like tabs have been converted to spaces here.

>  };
> 
>  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");

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.

> +
> +        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);

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

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

[Peter Fordham]

> 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?

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

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

[Jamie Iles]

On Tue, Jun 14, 2011 at 05:19:06PM -0700, Peter Fordham wrote:
> > 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?
> 
> 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.

> > 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

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

[Peter Fordham]

> 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.

>> > 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

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

[Jamie Iles]

On Wed, Jun 15, 2011 at 11:57:12AM -0700, Peter Fordham wrote:
> > 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.

> >> > 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

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

[Peter Fordham]

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

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

[Peter Fordham]

> 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.

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

[Arnd Bergmann]

On Wednesday 15 June 2011 21:09:45 Jamie Iles wrote:
> On Wed, Jun 15, 2011 at 11:57:12AM -0700, Peter Fordham wrote:
> > > 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

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

[Peter Fordham]

On 15 June 2011 12:36, Arnd Bergmann <arnd@arndb.de> 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

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

[Arnd Bergmann]

On Wednesday 15 June 2011 21:49:43 Peter Fordham wrote:
> On 15 June 2011 12:36, Arnd Bergmann <arnd@arndb.de> 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.

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.

> > 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

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

[Wim Van Sebroeck]

Hi Arnd,

> On Wednesday 15 June 2011 21:09:45 Jamie Iles wrote:
> > On Wed, Jun 15, 2011 at 11:57:12AM -0700, Peter Fordham wrote:
> > > > 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.

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

[Wim Van Sebroeck]

Hi All,

> > 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.

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

[Wim Van Sebroeck]

Hi All,

> On Tue, Jun 14, 2011 at 05:19:06PM -0700, Peter Fordham wrote:
> > > 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?
> > 
> > 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.
> 
> > > 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.