ar7_wdt: convert to become a platform driver

Hi Florian,

Review static struct miscdevice ar7_wdt_miscdev = { · line 288

Quoted 11 lines

This patch converts the ar7_wdt driver to become a platform driver. The AR7 SoC specific identification and base register calculation is performed by the board code, therefore we no longer need to have access to ar7_chip_id.

(quoted diff) +1 -1

@@ -298,22 +285,33 @@ static struct miscdevice ar7_wdt_miscdev = {
 	.fops		= &ar7_wdt_fops,
 };
 
-static int __init ar7_wdt_init(void)
+static int __init ar7_wdt_probe(struct platform_device *pdev)

Should be __devinit .

Review static struct miscdevice ar7_wdt_miscdev = { · line 293

Quoted 5 lines

(quoted diff) +5 -0

+static struct platform_driver ar7_wdt_driver = {
+	.driver.name = "ar7_wdt",
+	.probe = ar7_wdt_probe,
+	.remove = __devexit_p(ar7_wdt_remove),
+};

I prefer to have it as follows (so that the driver.owner field is also set):

static struct platform_driver ar7_wdt_driver = {
	.probe = ar7_wdt_probe,
	.remove = __devexit_p(ar7_wdt_remove),
	.driver = {
		.owner = THIS_MODULE,
		.name = "ar7_wdt",
	},
};

I suggest to also change the reboot notifier code into a platform shutdown method. You then get something like the attached patch (untested, uncompiled and I included above 2 remarks). For the rest: code is OK for me. After the __init to __devinit fix you can add my signed-off-by.

Kind regards,
Wim.

Hi Florian,

Forgot the attachment...

Kind regards,
Wim.

Quoted 37 lines

Hi Florian,

Review static struct miscdevice ar7_wdt_miscdev = { · line 288

Quoted 11 lines

This patch converts the ar7_wdt driver to become a platform driver. The AR7 SoC specific identification and base register calculation is performed by the board code, therefore we no longer need to have access to ar7_chip_id.

(quoted diff) +1 -1

@@ -298,22 +285,33 @@ static struct miscdevice ar7_wdt_miscdev = {
 	.fops		= &ar7_wdt_fops,
 };
 
-static int __init ar7_wdt_init(void)
+static int __init ar7_wdt_probe(struct platform_device *pdev)

Should be __devinit .

Review static struct miscdevice ar7_wdt_miscdev = { · line 293

Quoted 5 lines

(quoted diff) +5 -0

+static struct platform_driver ar7_wdt_driver = {
+	.driver.name = "ar7_wdt",
+	.probe = ar7_wdt_probe,
+	.remove = __devexit_p(ar7_wdt_remove),
+};

I prefer to have it as follows (so that the driver.owner field is also

set): static struct platform_driver ar7_wdt_driver = {
	.probe = ar7_wdt_probe,
	.remove = __devexit_p(ar7_wdt_remove),
	.driver = {
		.owner = THIS_MODULE,
		.name = "ar7_wdt",
	},
};

I suggest to also change the reboot notifier code into a platform shutdown method. You then get something like the attached patch (untested, uncompiled and I included above 2 remarks). For the rest: code is OK for me. After the __init to __devinit fix you can add my signed-off-by.

Quoted 3 lines

Kind regards,
Wim.

Hi Wim,

Le Saturday 18 July 2009 23:49:58 Wim Van Sebroeck, vous avez écrit :

Quoted 44 lines

Hi Florian,

Forgot the attachment...

Kind regards,
Wim.

Quoted 37 lines

Hi Florian,

drivers/watchdog/ar7_wdt.c static struct miscdevice ar7_wdt_miscdev = { · line 288

Quoted 11 lines

This patch converts the ar7_wdt driver to become a platform driver. The AR7 SoC specific identification and base register calculation is performed by the board code, therefore we no longer need to have access to ar7_chip_id.

drivers/watchdog/ar7_wdt.c +1 -1

@@ -298,22 +285,33 @@ static struct miscdevice ar7_wdt_miscdev = {
 	.fops		= &ar7_wdt_fops,
 };
 
-static int __init ar7_wdt_init(void)
+static int __init ar7_wdt_probe(struct platform_device *pdev)

Should be __devinit .

drivers/watchdog/ar7_wdt.c static struct miscdevice ar7_wdt_miscdev = { · line 342

Quoted 5 lines

drivers/watchdog/ar7_wdt.c +5 -0

+static struct platform_driver ar7_wdt_driver = {
+	.driver.name = "ar7_wdt",
+	.probe = ar7_wdt_probe,
+	.remove = __devexit_p(ar7_wdt_remove),
+};

I prefer to have it as follows (so that the driver.owner field is also

set): static struct platform_driver ar7_wdt_driver = {
	.probe = ar7_wdt_probe,
	.remove = __devexit_p(ar7_wdt_remove),
	.driver = {
		.owner = THIS_MODULE,
		.name = "ar7_wdt",
	},
};

I suggest to also change the reboot notifier code into a platform shutdown method. You then get something like the attached patch (untested, uncompiled and I included above 2 remarks). For the rest:
code is OK for me. After the __init to __devinit fix you can add my signed-off-by.

Thanks, patch updated and attached.

Quoted 2 lines

Quoted 2 lines

Kind regards,
Wim.

--
From: Florian Fainelli <[email protected]>
Subject: [PATCH 2/2 v2] ar7_wdt: convert to become a platform driver

This patch converts the ar7_wdt driver to become a platform driver. The AR7 SoC specific identification and base register calculation is performed by the board code, therefore we no longer need to have access to ar7_chip_id. We also remove the reboot notifier code to use the platform shutdown method as Wim suggested.

Signed-off-by Florian Fainelli <[email protected]>

Signed-off-by Wim Van Sebroeck <[email protected]>

--

drivers/watchdog/ar7_wdt.c +53 -53

index 2f8643e..82855b0 100644
@@ -28,9 +28,8 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/miscdevice.h>
+#include <linux/platform_device.h>
 #include <linux/watchdog.h>
-#include <linux/notifier.h>
-#include <linux/reboot.h>
 #include <linux/fs.h>
 #include <linux/ioport.h>
 #include <linux/io.h>

@@ -76,24 +75,10 @@ static unsigned expect_close;
 /* XXX currently fixed, allows max margin ~68.72 secs */
 #define prescale_value 0xffff
 
-/* Offset of the WDT registers */
-static unsigned long ar7_regs_wdt;
+/* Resource of the WDT registers */
+static struct resource *ar7_regs_wdt;
 /* Pointer to the remapped WDT IO space */
 static struct ar7_wdt *ar7_wdt;
-static void ar7_wdt_get_regs(void)
-{
-	u16 chip_id = ar7_chip_id();
-	switch (chip_id) {
-	case AR7_CHIP_7100:
-	case AR7_CHIP_7200:
-		ar7_regs_wdt = AR7_REGS_WDT;
-		break;
-	default:
-		ar7_regs_wdt = UR8_REGS_WDT;
-		break;
-	}
-}
-
 
 static void ar7_wdt_kick(u32 value)
 {

@@ -202,20 +187,6 @@ static int ar7_wdt_release(struct inode *inode, struct
 file *file) return 0;
 }
 
-static int ar7_wdt_notify_sys(struct notifier_block *this,
-			      unsigned long code, void *unused)
-{
-	if (code == SYS_HALT || code == SYS_POWER_OFF)
-		if (!nowayout)
-			ar7_wdt_disable_wdt();
-
-	return NOTIFY_DONE;
-}
-
-static struct notifier_block ar7_wdt_notifier = {
-	.notifier_call = ar7_wdt_notify_sys,
-};
-
 static ssize_t ar7_wdt_write(struct file *file, const char *data,
 			     size_t len, loff_t *ppos)
 {

@@ -299,56 +270,85 @@ static struct miscdevice ar7_wdt_miscdev = {
 	.fops		= &ar7_wdt_fops,
 };
 
-static int __init ar7_wdt_init(void)
+static int __devinit ar7_wdt_probe(struct platform_device *pdev)
 {
 	int rc;
 
 	spin_lock_init(&wdt_lock);
 
-	ar7_wdt_get_regs();
+	ar7_regs_wdt =
+		platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+	if (!ar7_regs_wdt) {
+		printk(KERN_ERR DRVNAME ": could not get registers resource\n");
+		rc = -ENODEV;
+		goto out;
+	}
 
-	if (!request_mem_region(ar7_regs_wdt, sizeof(struct ar7_wdt),
-							LONGNAME)) {
+	if (!request_mem_region(ar7_regs_wdt->start,
+				resource_size(ar7_regs_wdt), LONGNAME)) {
 		printk(KERN_WARNING DRVNAME ": watchdog I/O region busy\n");
-		return -EBUSY;
+		rc = -EBUSY;
+		goto out;
 	}
 
-	ar7_wdt = (struct ar7_wdt *)
-			ioremap(ar7_regs_wdt, sizeof(struct ar7_wdt));
+	ar7_wdt = ioremap(ar7_regs_wdt->start, resource_size(ar7_regs_wdt));
+	if (!ar7_wdt) {
+		printk(KERN_ERR DRVNAME ": could not ioremap registers\n");
+		rc = -ENXIO;
+		goto out;
+	}
 
 	ar7_wdt_disable_wdt();
 	ar7_wdt_prescale(prescale_value);
 	ar7_wdt_update_margin(margin);
 
-	rc = register_reboot_notifier(&ar7_wdt_notifier);
-	if (rc) {
-		printk(KERN_ERR DRVNAME
-			": unable to register reboot notifier\n");
-		goto out_alloc;
-	}
-
 	rc = misc_register(&ar7_wdt_miscdev);
 	if (rc) {
 		printk(KERN_ERR DRVNAME ": unable to register misc device\n");
-		goto out_register;
+		goto out_alloc;
 	}
 	goto out;
 
-out_register:
-	unregister_reboot_notifier(&ar7_wdt_notifier);
 out_alloc:
 	iounmap(ar7_wdt);
-	release_mem_region(ar7_regs_wdt, sizeof(struct ar7_wdt));
+	release_mem_region(ar7_regs_wdt->start, resource_size(ar7_regs_wdt));
 out:
 	return rc;
 }
 
-static void __exit ar7_wdt_cleanup(void)
+static int __devexit ar7_wdt_remove(struct platform_device *pdev)
 {
 	misc_deregister(&ar7_wdt_miscdev);
-	unregister_reboot_notifier(&ar7_wdt_notifier);
 	iounmap(ar7_wdt);
-	release_mem_region(ar7_regs_wdt, sizeof(struct ar7_wdt));
+	release_mem_region(ar7_regs_wdt->start, resource_size(ar7_regs_wdt));
+
+	return 0;
+}
+
+static void ar7_wdt_shutdown(struct platform_device *pdev)
+{
+	if (!nowayout)
+		ar7_wdt_disable_wdt();
+}
+
+static struct platform_driver ar7_wdt_driver = {
+	.probe = ar7_wdt_probe,
+	.remove = __devexit_p(ar7_wdt_remove),
+	.shutdown = ar7_wdt_shutdown,
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "ar7_wdt",
+	},
+};
+
+static int __init ar7_wdt_init(void)
+{
+	return platform_driver_register(&ar7_wdt_driver);
+}
+
+static void __exit ar7_wdt_cleanup(void)
+{
+	platform_driver_unregister(&ar7_wdt_driver);
 }
 
 module_init(ar7_wdt_init);

Quoted 10 lines

From: Florian Fainelli <[email protected]>
Subject: [PATCH 2/2 v2] ar7_wdt: convert to become a platform driver

This patch converts the ar7_wdt driver to become a platform driver. The AR7 SoC specific identification and base register calculation is performed by the board code, therefore we no longer need to have access to ar7_chip_id. We also remove the reboot notifier code to use the platform shutdown method as Wim suggested.

Quoted 2 lines

Signed-off-by Florian Fainelli <[email protected]>

Signed-off-by Wim Van Sebroeck <[email protected]>

Any news on this patch ?

Hi Florian,

Quoted 10 lines

Quoted 10 lines

From: Florian Fainelli <[email protected]>
Subject: [PATCH 2/2 v2] ar7_wdt: convert to become a platform driver

This patch converts the ar7_wdt driver to become a platform driver. The AR7 SoC specific identification and base register calculation is performed by the board code, therefore we no longer need to have access to ar7_chip_id. We also remove the reboot notifier code to use the platform shutdown method as Wim suggested.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Florian Fainelli <[email protected]>

Signed-off-by Wim Van Sebroeck <[email protected]>

Any news on this patch ?

This one was ok for me. I think we agreed that Ralf would take it up in his tree. I can also take it up in my next tree still.

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

From: Florian Fainelli <[email protected]>
Subject: [PATCH 2/2 v2] ar7_wdt: convert to become a platform driver

This patch converts the ar7_wdt driver to become a platform driver. The AR7 SoC specific identification and base register calculation is performed by the board code, therefore we no longer need to have access to ar7_chip_id. We also remove the reboot notifier code to use the platform shutdown method as Wim suggested.

Signed-off-by Florian Fainelli <[email protected]>

Signed-off-by Wim Van Sebroeck <[email protected]>

Quoted 4 lines

Quoted 1 line

Any news on this patch ?

This one was ok for me. I think we agreed that Ralf would take it up in his tree. I can also take it up in my next tree still.

Oh, I did not understand that sorry, I thought Ralf would take the first one which is MIPS-specific.

Hi Florian,

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

From: Florian Fainelli <[email protected]>
Subject: [PATCH 2/2 v2] ar7_wdt: convert to become a platform driver

This patch converts the ar7_wdt driver to become a platform driver. The AR7 SoC specific identification and base register calculation is performed by the board code, therefore we no longer need to have access to ar7_chip_id. We also remove the reboot notifier code to use the platform shutdown method as Wim suggested.

Quoted 2 lines

Signed-off-by Florian Fainelli <[email protected]>

Signed-off-by Wim Van Sebroeck <[email protected]>

Any news on this patch ?

Quoted 5 lines

Quoted 2 lines

This one was ok for me. I think we agreed that Ralf would take it up in his tree. I can also take it up in my next tree still.

Oh, I did not understand that sorry, I thought Ralf would take the first one which is MIPS-specific.

I added the second patch to my tree, but saw that the error handling on probe could be improved. Can you test attached patch?