mpc52xx/wdt: WDT uses GPT api

PATCH In review Albrecht Dreß 5 participants 10 messages Nov 10, 2009 – Dec 8, 2009 4 messages not in the archive Watchdog device drivers

lore

Share & tools

lore

Albrecht Dreß

author

2009-11-10 19:43 UTC #0 lore raw

PATCH 3/3 mpc52xx/wdt: WDT uses GPT api

Use the MPC5200 GPT api for the WDT which drastically simplifies this file.

Signed-off-by Albrecht Dreß <[email protected]>

---

 drivers/watchdog/mpc5200_wdt.c |  246 +++++++++++-----------------------------
 1 files changed, 65 insertions(+), 181 deletions(-)

drivers/watchdog/mpc5200_wdt.c +65 ~~-181~~

index fa9c47c..5bb553c 100644
@@ -2,25 +2,16 @@
 #include <linux/module.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>
-#include <linux/io.h>
-#include <linux/spinlock.h>
 #include <linux/of_platform.h>
 #include <linux/uaccess.h>
 #include <asm/mpc52xx.h>
 
 
-#define GPT_MODE_WDT		(1 << 15)
-#define GPT_MODE_CE		(1 << 12)
-#define GPT_MODE_MS_TIMER	(0x4)
-
+#define WDT_IDENTITY        "mpc5200 watchdog on GPT0"
 
 struct mpc5200_wdt {
-	unsigned count;	/* timer ticks before watchdog kicks in */
-	long ipb_freq;
-	struct miscdevice miscdev;
-	struct resource mem;
-	struct mpc52xx_gpt __iomem *regs;
-	spinlock_t io_lock;
+	int timeout;
+	struct mpc52xx_gpt_priv *timer;
 };
 
 /* is_active stores wether or not the /dev/watchdog device is opened */

@@ -32,80 +23,33 @@ static unsigned long is_active;
 static struct mpc5200_wdt *wdt_global;
 
 
-/* helper to calculate timeout in timer counts */
-static void mpc5200_wdt_set_timeout(struct mpc5200_wdt *wdt, int timeout)
-{
-	/* use biggest prescaler of 64k */
-	wdt->count = (wdt->ipb_freq + 0xffff) / 0x10000 * timeout;
-
-	if (wdt->count > 0xffff)
-		wdt->count = 0xffff;
-}
-/* return timeout in seconds (calculated from timer count) */
-static int mpc5200_wdt_get_timeout(struct mpc5200_wdt *wdt)
-{
-	return wdt->count * 0x10000 / wdt->ipb_freq;
-}
-
-
-/* watchdog operations */
-static int mpc5200_wdt_start(struct mpc5200_wdt *wdt)
-{
-	spin_lock(&wdt->io_lock);
-	/* disable */
-	out_be32(&wdt->regs->mode, 0);
-	/* set timeout, with maximum prescaler */
-	out_be32(&wdt->regs->count, 0x0 | wdt->count);
-	/* enable watchdog */
-	out_be32(&wdt->regs->mode, GPT_MODE_CE | GPT_MODE_WDT |
-						GPT_MODE_MS_TIMER);
-	spin_unlock(&wdt->io_lock);
-
-	return 0;
-}
-static int mpc5200_wdt_ping(struct mpc5200_wdt *wdt)
-{
-	spin_lock(&wdt->io_lock);
-	/* writing A5 to OCPW resets the watchdog */
-	out_be32(&wdt->regs->mode, 0xA5000000 |
-				(0xffffff & in_be32(&wdt->regs->mode)));
-	spin_unlock(&wdt->io_lock);
-	return 0;
-}
-static int mpc5200_wdt_stop(struct mpc5200_wdt *wdt)
-{
-	spin_lock(&wdt->io_lock);
-	/* disable */
-	out_be32(&wdt->regs->mode, 0);
-	spin_unlock(&wdt->io_lock);
-	return 0;
-}
-
-
 /* file operations */
 static ssize_t mpc5200_wdt_write(struct file *file, const char __user *data,
-		size_t len, loff_t *ppos)
+				 size_t len, loff_t *ppos)
 {
 	struct mpc5200_wdt *wdt = file->private_data;
-	mpc5200_wdt_ping(wdt);
+	mpc52xx_gpt_wdt_ping(wdt->timer);
 	return 0;
 }
+
 static struct watchdog_info mpc5200_wdt_info = {
 	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
-	.identity	= "mpc5200 watchdog on GPT0",
+	.identity	= WDT_IDENTITY,
 };
+
 static long mpc5200_wdt_ioctl(struct file *file, unsigned int cmd,
-							unsigned long arg)
+			      unsigned long arg)
 {
 	struct mpc5200_wdt *wdt = file->private_data;
 	int __user *data = (int __user *)arg;
 	int timeout;
+	u64 real_timeout;
 	int ret = 0;
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
 		ret = copy_to_user(data, &mpc5200_wdt_info,
-						sizeof(mpc5200_wdt_info));
+				   sizeof(mpc5200_wdt_info));
 		if (ret)
 			ret = -EFAULT;
 		break;

@@ -116,19 +60,23 @@ static long mpc5200_wdt_ioctl(struct file *file, unsigned int cmd,
 		break;
 
 	case WDIOC_KEEPALIVE:
-		mpc5200_wdt_ping(wdt);
+		mpc52xx_gpt_wdt_ping(wdt->timer);
 		break;
 
 	case WDIOC_SETTIMEOUT:
 		ret = get_user(timeout, data);
 		if (ret)
 			break;
-		mpc5200_wdt_set_timeout(wdt, timeout);
-		mpc5200_wdt_start(wdt);
+		ret = mpc52xx_gpt_wdt_start(wdt->timer, timeout);
+		if (ret)
+			break;
+		wdt->timeout = timeout;
 		/* fall through and return the timeout */
 
 	case WDIOC_GETTIMEOUT:
-		timeout = mpc5200_wdt_get_timeout(wdt);
+		real_timeout = mpc52xx_gpt_timer_period(wdt->timer);
+		do_div(real_timeout, 1000000000);
+		timeout = (int) real_timeout;
 		ret = put_user(timeout, data);
 		break;

@@ -140,154 +88,90 @@ static long mpc5200_wdt_ioctl(struct file *file, unsigned int cmd,
 
 static int mpc5200_wdt_open(struct inode *inode, struct file *file)
 {
+	int ret;
+
 	/* /dev/watchdog can only be opened once */
 	if (test_and_set_bit(0, &is_active))
 		return -EBUSY;
 
 	/* Set and activate the watchdog */
-	mpc5200_wdt_set_timeout(wdt_global, 30);
-	mpc5200_wdt_start(wdt_global);
+	ret = mpc52xx_gpt_wdt_start(wdt_global->timer, 30);
+	if (ret) {
+		clear_bit(0, &is_active);
+		return ret;
+	}
+	wdt_global->timeout = 30;
+
 	file->private_data = wdt_global;
 	return nonseekable_open(inode, file);
 }
+
 static int mpc5200_wdt_release(struct inode *inode, struct file *file)
 {
-#if WATCHDOG_NOWAYOUT == 0
+#if !defined(CONFIG_WATCHDOG_NOWAYOUT)
 	struct mpc5200_wdt *wdt = file->private_data;
-	mpc5200_wdt_stop(wdt);
-	wdt->count = 0;		/* == disabled */
+	mpc52xx_gpt_wdt_stop(wdt->timer);
+	wdt->timeout = 0;		/* == disabled */
 #endif
 	clear_bit(0, &is_active);
 	return 0;
 }
 
 static const struct file_operations mpc5200_wdt_fops = {
-	.owner	= THIS_MODULE,
-	.write	= mpc5200_wdt_write,
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.write		= mpc5200_wdt_write,
 	.unlocked_ioctl	= mpc5200_wdt_ioctl,
-	.open	= mpc5200_wdt_open,
-	.release = mpc5200_wdt_release,
+	.open		= mpc5200_wdt_open,
+	.release	= mpc5200_wdt_release,
+};
+
+static struct miscdevice mpc5200_wdt_miscdev = {
+	.minor		= WATCHDOG_MINOR,
+	.name		= "watchdog",
+	.fops		= &mpc5200_wdt_fops,
 };
 
 /* module operations */
-static int mpc5200_wdt_probe(struct of_device *op,
-					const struct of_device_id *match)
+static int __init mpc5200_wdt_init(void)
 {
-	struct mpc5200_wdt *wdt;
 	int err;
-	const void *has_wdt;
-	int size;
+	struct mpc52xx_gpt_priv *timer;
 
-	has_wdt = of_get_property(op->node, "has-wdt", NULL);
-	if (!has_wdt)
-		has_wdt = of_get_property(op->node, "fsl,has-wdt", NULL);
-	if (!has_wdt)
+	/* grab the watchdog-capable gpt */
+	timer = mpc52xx_gpt_wdt_probe();
+	if (!timer) {
+		pr_err(WDT_IDENTITY ": probing failed\n");
 		return -ENODEV;
+	}
 
-	wdt = kzalloc(sizeof(*wdt), GFP_KERNEL);
-	if (!wdt)
+	wdt_global = kzalloc(sizeof(struct mpc5200_wdt), GFP_KERNEL);
+	if (!wdt_global) {
+		mpc52xx_gpt_wdt_release(timer);
 		return -ENOMEM;
-
-	wdt->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
-
-	err = of_address_to_resource(op->node, 0, &wdt->mem);
-	if (err)
-		goto out_free;
-	size = wdt->mem.end - wdt->mem.start + 1;
-	if (!request_mem_region(wdt->mem.start, size, "mpc5200_wdt")) {
-		err = -ENODEV;
-		goto out_free;
-	}
-	wdt->regs = ioremap(wdt->mem.start, size);
-	if (!wdt->regs) {
-		err = -ENODEV;
-		goto out_release;
 	}
-
-	dev_set_drvdata(&op->dev, wdt);
-	spin_lock_init(&wdt->io_lock);
-
-	wdt->miscdev = (struct miscdevice) {
-		.minor	= WATCHDOG_MINOR,
-		.name	= "watchdog",
-		.fops	= &mpc5200_wdt_fops,
-		.parent = &op->dev,
-	};
-	wdt_global = wdt;
-	err = misc_register(&wdt->miscdev);
+	wdt_global->timer = timer;
+	err = misc_register(&mpc5200_wdt_miscdev);
 	if (!err)
-		return 0;
-
-	iounmap(wdt->regs);
-out_release:
-	release_mem_region(wdt->mem.start, size);
-out_free:
-	kfree(wdt);
+		pr_info(WDT_IDENTITY ": registered\n");
+	else {
+		mpc52xx_gpt_wdt_release(timer);
+		kfree(wdt_global);
+	}
 	return err;
 }
 
-static int mpc5200_wdt_remove(struct of_device *op)
-{
-	struct mpc5200_wdt *wdt = dev_get_drvdata(&op->dev);
-
-	mpc5200_wdt_stop(wdt);
-	misc_deregister(&wdt->miscdev);
-	iounmap(wdt->regs);
-	release_mem_region(wdt->mem.start, wdt->mem.end - wdt->mem.start + 1);
-	kfree(wdt);
-
-	return 0;
-}
-static int mpc5200_wdt_suspend(struct of_device *op, pm_message_t state)
-{
-	struct mpc5200_wdt *wdt = dev_get_drvdata(&op->dev);
-	mpc5200_wdt_stop(wdt);
-	return 0;
-}
-static int mpc5200_wdt_resume(struct of_device *op)
-{
-	struct mpc5200_wdt *wdt = dev_get_drvdata(&op->dev);
-	if (wdt->count)
-		mpc5200_wdt_start(wdt);
-	return 0;
-}
-static int mpc5200_wdt_shutdown(struct of_device *op)
-{
-	struct mpc5200_wdt *wdt = dev_get_drvdata(&op->dev);
-	mpc5200_wdt_stop(wdt);
-	return 0;
-}
-
-static struct of_device_id mpc5200_wdt_match[] = {
-	{ .compatible = "mpc5200-gpt", },
-	{ .compatible = "fsl,mpc5200-gpt", },
-	{},
-};
-static struct of_platform_driver mpc5200_wdt_driver = {
-	.owner		= THIS_MODULE,
-	.name		= "mpc5200-gpt-wdt",
-	.match_table	= mpc5200_wdt_match,
-	.probe		= mpc5200_wdt_probe,
-	.remove		= mpc5200_wdt_remove,
-	.suspend	= mpc5200_wdt_suspend,
-	.resume		= mpc5200_wdt_resume,
-	.shutdown	= mpc5200_wdt_shutdown,
-};
-
-
-static int __init mpc5200_wdt_init(void)
-{
-	return of_register_platform_driver(&mpc5200_wdt_driver);
-}
-
 static void __exit mpc5200_wdt_exit(void)
 {
-	of_unregister_platform_driver(&mpc5200_wdt_driver);
+	mpc52xx_gpt_wdt_release(wdt_global->timer);
+	misc_deregister(&mpc5200_wdt_miscdev);
+	kfree(wdt_global);
 }
 
 module_init(mpc5200_wdt_init);
 module_exit(mpc5200_wdt_exit);
 
-MODULE_AUTHOR("Domen Puncer <[email protected]>");
+MODULE_AUTHOR("Domen Puncer <[email protected]>, "
+	      "Albrecht Dreß <[email protected]>");
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Grant Likely

2009-11-10 19:59 UTC #1 lore raw

Quoted 7 lines · Albrecht Dreß

Use the MPC5200 GPT api for the WDT which drastically simplifies this file.
Signed-off-by Albrecht Dreß <[email protected]>
---
drivers/watchdog/mpc5200_wdt.c | 246 +++++++++++-----------------------------
 1 files changed, 65 insertions(+), 181 deletions(-)

Can the WDT functionality just be merged entirely into arch/powerpc/platforms/52xx/mpc52xx_gpt.c, eliminating the need for this file entirely? I think I'd rather have all the GPT "built in" behaviour handled by a single driver.

Signature · 2 lines

Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Albrecht Dreß

author

2009-11-10 20:26 UTC #2 lore raw

Hi Grant:

Am 10.11.09 20:59 schrieb(en) Grant Likely:

Quoted 6 lines · Albrecht Dreß

Quoted 5 lines
Use the MPC5200 GPT api for the WDT which drastically simplifies this file.
Signed-off-by Albrecht Dreß <[email protected]>
---

Quoted 8 lines

Quoted 2 lines
drivers/watchdog/mpc5200_wdt.c | 246 +++++++++++-----------------------------
 1 files changed, 65 insertions(+), 181 deletions(-)
Can the WDT functionality just be merged entirely into arch/powerpc/platforms/52xx/mpc52xx_gpt.c, eliminating the need for this file entirely? I think I'd rather have all the GPT "built in" behaviour handled by a single driver.

I also thought about it, as it has IMHO the cleaner code, and it would have the extra benefit that the gpt-wdt api doesn't need to be public.

However, the reasons I hesitated to do so are:
- I don't want to remove a file someone else wrote (even it doesn't work);
- WDT code is shifted from drivers/watchdog to arch/powerpc/platforms/52xx which might not be the "logical" place from the directory layout's pov;
- a file living in arch/powerpc/platforms/52xx depends upon config options set from drivers/watchdog/Kconfig which may be confusing.

You see these are more political/cosmetical questions, so I would prefer to leave the decision to the maintainers (i.e. you and Wim). Preparing a fully merged driver is actually a matter of minutes!

Thanks,
Albrecht.

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

Grant Likely

2009-11-10 21:07 UTC #3 lore raw

Quoted 22 lines · Albrecht Dreß

Hi Grant:
Am 10.11.09 20:59 schrieb(en) Grant Likely:
Quoted 14 lines
Quoted 9 lines · Albrecht Dreß
Use the MPC5200 GPT api for the WDT which drastically simplifies this file.
Signed-off-by Albrecht Dreß <[email protected]>
---
drivers/watchdog/mpc5200_wdt.c | 246
+++++++++++-----------------------------
 1 files changed, 65 insertions(+), 181 deletions(-)
Quoted 4 lines
Can the WDT functionality just be merged entirely into arch/powerpc/platforms/52xx/mpc52xx_gpt.c, eliminating the need for this file entirely? I think I'd rather have all the GPT "built in" behaviour handled by a single driver.

Quoted 5 lines

I also thought about it, as it has IMHO the cleaner code, and it would have the extra benefit that the gpt-wdt api doesn't need to be public.
However, the reasons I hesitated to do so are:
- I don't want to remove a file someone else wrote (even it doesn't work);

Shouldn't be a problem, and I'll handle the fallout if it is.

Quoted 2 lines

- WDT code is shifted from drivers/watchdog to arch/powerpc/platforms/52xx which might not be the "logical" place from the directory layout's pov;

There is precedence of this in the past, particularly on arch or platform specific hardware drivers and multifunction devices. (Heck, that's almost entirely what arch/powerpc/sysdev is). I'm not concerned.

Quoted 2 lines

- a file living in arch/powerpc/platforms/52xx depends upon config options set from drivers/watchdog/Kconfig which may be confusing.

I'm not concerned about this either.

Quoted 3 lines

You see these are more political/cosmetical questions, so I would prefer to leave the decision to the maintainers (i.e. you and Wim). Preparing a fully merged driver is actually a matter of minutes!

Do it. I'll champion getting it in. Wim, do you have any issues with this?

Signature · 2 lines

Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Albrecht Dre�

author

2009-11-11 08:32 UTC #4 lore raw

Hi Grant:

O.k., thanks for your comments. If Wim doesn't have any objections to it, I will provide a merged patch. One consequence I forgot to mention is that we loose the ability to build the wdt support as module, but I don't think it's a real problem.

I think we still should keep the kernel config option enable/disable the wdt support, which would mask out the wdt code if disabled. Is that ok for you?

Thanks, Albrecht.

----- Original Nachricht ----

Von:     Grant Likely <[email protected]>
An:      Albrecht Dreß <[email protected]>
Datum:   10.11.2009 22:07
Betreff: Re: [PATCH 3/3] mpc52xx/wdt: WDT uses GPT api

Quoted 52 lines

Quoted 5 lines · Albrecht Dreß
Hi Grant:
Am 10.11.09 20:59 schrieb(en) Grant Likely:
Quoted 2 lines
On Tue, Nov 10, 2009 at 12:43 PM, Albrecht Dreß
<[email protected]>
Quoted 12 lines
Quoted 12 lines
Quoted 9 lines · wrote:
Use the MPC5200 GPT api for the WDT which drastically simplifies this file.
Signed-off-by Albrecht Dreß <[email protected]>
---
drivers/watchdog/mpc5200_wdt.c | 246
+++++++++++-----------------------------
 1 files changed, 65 insertions(+), 181 deletions(-)
Quoted 6 lines
Quoted 4 lines
Can the WDT functionality just be merged entirely into arch/powerpc/platforms/52xx/mpc52xx_gpt.c, eliminating the need for this file entirely? I think I'd rather have all the GPT "built in" behaviour handled by a single driver.
I also thought about it, as it has IMHO the cleaner code, and it would
have
Quoted 4 lines
the extra benefit that the gpt-wdt api doesn't need to be public.
However, the reasons I hesitated to do so are:
- I don't want to remove a file someone else wrote (even it doesn't
work);
Shouldn't be a problem, and I'll handle the fallout if it is.
Quoted 1 line
- WDT code is shifted from drivers/watchdog to
arch/powerpc/platforms/52xx
Quoted 1 line
which might not be the "logical" place from the directory layout's pov;
There is precedence of this in the past, particularly on arch or platform specific hardware drivers and multifunction devices. (Heck, that's almost entirely what arch/powerpc/sysdev is). I'm not concerned.
Quoted 1 line
- a file living in arch/powerpc/platforms/52xx depends upon config
options
Quoted 1 line
set from drivers/watchdog/Kconfig which may be confusing.
I'm not concerned about this either.
Quoted 1 line
You see these are more political/cosmetical questions, so I would prefer
to

Quoted 17 lines

Quoted 1 line
leave the decision to the maintainers (i.e. you and Wim). Preparing a
fully
Quoted 1 line
merged driver is actually a matter of minutes!
Do it. I'll champion getting it in. Wim, do you have any issues with this?
g.
Signature · 6 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

Jetzt NEU: Do it youself E-Cards bei Arcor.de!
Stellen Sie Ihr eigenes Unikat zusammen und machen Sie dem Empfänger eine ganz persönliche Freude! E-Card Marke Eigenbau: HIER KLICKEN: http://www.arcor.de/rd/footer.ecard

A message that is not in the archive (sent off-list or not yet received): 16246194.1257928342802.JavaMail.ngmail-hrWuGRo80rRiK3oW1guXusM6rOWSkUom@public.gmane.org

Grant Likely

2009-11-11 20:18 UTC #6 lore raw

Quoted 3 lines · Albrecht Dreß

Hi Grant:
O.k., thanks for your comments. If Wim doesn't have any objections to it, I will provide a merged patch. One consequence I forgot to mention is that we loose the ability to build the wdt support as module, but I don't think it's a real problem.

I'm not too worried about this. It is also potentially possible to rework the entire GPT driver as a module. But there are non-trivial architecture fiddly bits needed to make it safe to load IRQ controllers in a module.

Quoted 1 line

I think we still should keep the kernel config option enable/disable the wdt support, which would mask out the wdt code if disabled. Is that ok for you?

Yup.

Signature · 2 lines

Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Wim Van Sebroeck

2009-11-12 21:36 UTC #7 lore raw

In reply to Albrecht Dreß

Hi All,

Quoted 5 lines

Quoted 4 lines
Can the WDT functionality just be merged entirely into arch/powerpc/platforms/52xx/mpc52xx_gpt.c, eliminating the need for this file entirely? I think I'd rather have all the GPT "built in" behaviour handled by a single driver.

Quoted 8 lines

I also thought about it, as it has IMHO the cleaner code, and it would have the extra benefit that the gpt-wdt api doesn't need to be public.
However, the reasons I hesitated to do so are:
- I don't want to remove a file someone else wrote (even it doesn't work);
- WDT code is shifted from drivers/watchdog to arch/powerpc/platforms/52xx which might not be the "logical" place from the directory layout's pov;
- a file living in arch/powerpc/platforms/52xx depends upon config options set from drivers/watchdog/Kconfig which may be confusing.
You see these are more political/cosmetical questions, so I would prefer to leave the decision to the maintainers (i.e. you and Wim). Preparing a fully merged driver is actually a matter of minutes!

My opinion: it is harder to maintain the watchdog code if it is being moved away from drivers/watchdog.
I need to check the code before I comment any further on this, but my first thought is: why don't you do it with platform resources like other devices are doing? This way you can keep the platform code under arch and the watchdog itself under drivers/watchdog/. You can look at the following drivers as an example: adx_wdt.c ar7_wdt.c at32ap700x_wdt.c coh901327_wdt.c davinci_wdt.c mpcore_wdt.c mv64x60_wdt.c nuc900_wdt.c omap_wdt.c pnx4008_wdt.c rc32434_wdt.c s3c2410_wdt.c txx9wdt.c .

Kind regards,
Wim.

A message that is not in the archive (sent off-list or not yet received): 20091112213630.GE19336-flHiHfN8CTwhDM6iD19NGrNAH6kLmebB@public.gmane.org

Grant Likely

2009-11-13 06:21 UTC #9 lore raw

Quoted 14 lines · Wim Van Sebroeck

Hi All,
Quoted 12 lines
Quoted 4 lines
Can the WDT functionality just be merged entirely into arch/powerpc/platforms/52xx/mpc52xx_gpt.c, eliminating the need for this file entirely? I think I'd rather have all the GPT "built in" behaviour handled by a single driver.
I also thought about it, as it has IMHO the cleaner code, and it would have the extra benefit that the gpt-wdt api doesn't need to be public.
However, the reasons I hesitated to do so are:
- I don't want to remove a file someone else wrote (even it doesn't work);
- WDT code is shifted from drivers/watchdog to arch/powerpc/platforms/52xx which might not be the "logical" place from the directory layout's pov;
- a file living in arch/powerpc/platforms/52xx depends upon config options set from drivers/watchdog/Kconfig which may be confusing.

Quoted 4 lines

Quoted 1 line
You see these are more political/cosmetical questions, so I would prefer to leave the decision to the maintainers (i.e. you and Wim). Preparing a fully merged driver is actually a matter of minutes!
My opinion: it is harder to maintain the watchdog code if it is being moved away from drivers/watchdog.
I need to check the code before I comment any further on this, but my first thought is: why don't you do it with platform resources like other devices are doing? This way you can keep the platform code under arch and the watchdog itself under drivers/watchdog/. You can look at the following drivers as an example: adx_wdt.c ar7_wdt.c at32ap700x_wdt.c coh901327_wdt.c davinci_wdt.c mpcore_wdt.c mv64x60_wdt.c nuc900_wdt.c omap_wdt.c pnx4008_wdt.c rc32434_wdt.c s3c2410_wdt.c txx9wdt.c .

In actual fact, it is a single device with multiple functions, some of which can be used at the same time. The driver for the device determines what the device instance supports and registers the appropriate interfaces. There is a GPIO controller, a PWM controller, a general purpose timer, and the watchdog. Because of the multifunction nature of the device, there are subtle interactions between the functions that the driver needs to maintain. I don't want to export functions from the driver which will only be used by a watchdog instance. I also don't want the added code and complexity of a secondary probe path. It is simpler and less code to roll all the behaviour up into the one driver single driver that gets probed once.

Quoted 1 line

From the maintenance perspective, having the main driver in

arch/powerpc and the watchdog bit in drivers/watchdog doesn't really help much anyway because anything that changes the internal driver API (between the core and watchdog bits) will require cross-maintainer changes. ie. do changes go through my tree because they touch arch/powerpc, or do they go through yours because they touch drivers/watchdog? I'd much rather all the internal details be contained within a single driver.

Besides, there is already precedence for very arch-specific drivers living under arch/*/. find ./arch -name *gpio*

Cheers,
g.

Signature · 2 lines

Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

A message that is not in the archive (sent off-list or not yet received): fa686aa40911122221s24efdcc9k1daf63b600e7b429-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Wim Van Sebroeck

2009-12-08 20:48 UTC #11 lore raw

Hi Grant,

Sorry for the delay, I need to deliver a project in a weeks time...

Quoted 12 lines

In actual fact, it is a single device with multiple functions, some of which can be used at the same time. The driver for the device determines what the device instance supports and registers the appropriate interfaces. There is a GPIO controller, a PWM controller, a general purpose timer, and the watchdog. Because of the multifunction nature of the device, there are subtle interactions between the functions that the driver needs to maintain. I don't want to export functions from the driver which will only be used by a watchdog instance. I also don't want the added code and complexity of a secondary probe path. It is simpler and less code to roll all the behaviour up into the one driver single driver that gets probed once.

Quoted 8 lines

Quoted 1 line
From the maintenance perspective, having the main driver in
arch/powerpc and the watchdog bit in drivers/watchdog doesn't really help much anyway because anything that changes the internal driver API (between the core and watchdog bits) will require cross-maintainer changes. ie. do changes go through my tree because they touch arch/powerpc, or do they go through yours because they touch drivers/watchdog? I'd much rather all the internal details be contained within a single driver.

Your argument about maintenance is the same one as I have: If all watchdog driver pieces are under drivers/watchdog then it's easier for me to maintain them. (Definitely if we are doing clean-up work and API changes).

Quoted 2 lines

Besides, there is already precedence for very arch-specific drivers living under arch/*/. find ./arch -name *gpio*

But in this case: I know where to find them and I will keep a mental note about this one. And yes indeed some very arch specific drivers can reside under arch/*/* .

So please go ahead with pulling this one in a single driver.

Kind regards,
Wim.

A message that is not in the archive (sent off-list or not yet received): 20091208204854.GE4291-flHiHfN8CTwhDM6iD19NGrNAH6kLmebB@public.gmane.org

Grant Likely

2009-12-08 21:21 UTC #13 lore raw

Quoted 3 lines · Wim Van Sebroeck

Hi Grant,
Sorry for the delay, I need to deliver a project in a weeks time...

understood.

Quoted 21 lines

Quoted 12 lines
In actual fact, it is a single device with multiple functions, some of which can be used at the same time. The driver for the device determines what the device instance supports and registers the appropriate interfaces. There is a GPIO controller, a PWM controller, a general purpose timer, and the watchdog. Because of the multifunction nature of the device, there are subtle interactions between the functions that the driver needs to maintain. I don't want to export functions from the driver which will only be used by a watchdog instance. I also don't want the added code and complexity of a secondary probe path. It is simpler and less code to roll all the behaviour up into the one driver single driver that gets probed once.
Quoted 8 lines
Quoted 1 line
From the maintenance perspective, having the main driver in
arch/powerpc and the watchdog bit in drivers/watchdog doesn't really help much anyway because anything that changes the internal driver API (between the core and watchdog bits) will require cross-maintainer changes. ie. do changes go through my tree because they touch arch/powerpc, or do they go through yours because they touch drivers/watchdog? I'd much rather all the internal details be contained within a single driver.

Quoted 3 lines

Your argument about maintenance is the same one as I have: If all watchdog driver pieces are under drivers/watchdog then it's easier for me to maintain them. (Definitely if we are doing clean-up work and API changes).

:-)

Quoted 8 lines

Quoted 2 lines
Besides, there is already precedence for very arch-specific drivers living under arch/*/. find ./arch -name *gpio*
But in this case: I know where to find them and I will keep a mental note about this one. And yes indeed some very arch specific drivers can reside under arch/*/* .
So please go ahead with pulling this one in a single driver.

Great, Thanks Wim.

Signature · 2 lines

Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.