[PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Felipe Balbi]

From: Felipe Balbi <felipe.balbi@nokia.com>

Get rid of cpu conditional code.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 arch/arm/plat-omap/devices.c      |   65 +++++++++++++++++--
 drivers/watchdog/omap_wdt.c       |  127 +++++++++++++------------------------
 include/linux/watchdog/omap_wdt.h |   29 +++++++++
 3 files changed, 132 insertions(+), 89 deletions(-)

diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
index f1eaa44..d756743 100644
--- a/arch/arm/plat-omap/devices.c
+++ b/arch/arm/plat-omap/devices.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
+#include <linux/watchdog/omap_wdt.h>
 
 #include <mach/hardware.h>
 #include <asm/io.h>
@@ -460,6 +461,51 @@ static struct resource wdt_resources[] = {
 	},
 };
 
+static int omap_wdt_set_clock(struct omap_wdt_dev *wdev, int status)
+{
+	if (status) {
+		if (cpu_is_omap16xx())
+			clk_disable(wdev->fck);
+
+		if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+			clk_disable(wdev->ick);
+			clk_disable(wdev->fck);
+		}
+	} else {
+		if (cpu_is_omap16xx())
+			clk_disable(wdev->fck);
+
+		if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+			clk_disable(wdev->ick);
+			clk_disable(wdev->fck);
+		}
+	}
+
+	return 0;
+}
+
+static int omap_wdt_get_bootstatus(void)
+{
+	if (cpu_is_omap16xx())
+		return __raw_readw(ARM_SYSST);
+
+#if 0
+	/* REVISIT: When the patch introducing the following function
+	 * gets merged upstream, uncomment this code to enable omap2
+	 * support on omap watchdog driver.
+	 */
+	if (cpu_is_omap24xx())
+		return omap_prcm_get_reset_sources();
+#endif
+
+	return 0;
+}
+
+static struct omap_wdt_platform_data omap_wdt_pdata = {
+	.set_clock	= omap_wdt_set_clock,
+	.get_bootstatus	= omap_wdt_get_bootstatus,
+};
+
 static struct platform_device omap_wdt_device = {
 	.name	   = "omap_wdt",
 	.id	     = -1,
@@ -469,17 +515,26 @@ static struct platform_device omap_wdt_device = {
 
 static void omap_init_wdt(void)
 {
-	if (cpu_is_omap16xx())
+	if (cpu_is_omap16xx()) {
+		omap_wdt_pdata.fck = "armwdt_ck";
 		wdt_resources[0].start = 0xfffeb000;
-	else if (cpu_is_omap2420())
+	} else if (cpu_is_omap2420()) {
+		omap_wdt_pdata.fck = "mpu_wdt_ick";
+		omap_wdt_pdata.ick = "mpu_wdt_fck";
 		wdt_resources[0].start = 0x48022000; /* WDT2 */
-	else if (cpu_is_omap2430())
+	} else if (cpu_is_omap2430()) {
+		omap_wdt_pdata.fck = "mpu_wdt_ick";
+		omap_wdt_pdata.ick = "mpu_wdt_fck";
 		wdt_resources[0].start = 0x49016000; /* WDT2 */
-	else if (cpu_is_omap343x())
+	} else if (cpu_is_omap343x()) {
+		omap_wdt_pdata.fck = "wdt2_ick";
+		omap_wdt_pdata.ick = "wdt2_fck";
 		wdt_resources[0].start = 0x48314000; /* WDT2 */
-	else
+	} else {
 		return;
+	}
 
+	omap_wdt_device.dev.platform_data = &omap_wdt_pdata;
 	wdt_resources[0].end = wdt_resources[0].start + 0x4f;
 
 	(void) platform_device_register(&omap_wdt_device);
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 7700a0a..52c597c 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -48,23 +48,20 @@
 
 #include <mach/prcm.h>
 
-static struct platform_device *omap_wdt_dev;
-
 static unsigned timer_margin;
 module_param(timer_margin, uint, 0);
 MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
 
+static struct platform_device *omap_wdt_dev;
 static unsigned int wdt_trgr_pattern = 0x1234;
-struct omap_wdt_dev {
-	void __iomem    *base;          /* physical */
-	struct device   *dev;
-	int             omap_wdt_users;
-	struct clk      *armwdt_ck;
-	struct clk      *mpu_wdt_ick;
-	struct clk      *mpu_wdt_fck;
-	struct resource *mem;
-	struct miscdevice omap_wdt_miscdev;
-};
+
+static inline int omap_wdt_set_clock(struct omap_wdt_dev *wdev, int status)
+{
+	if (wdev->set_clock)
+		return wdev->set_clock(wdev, status);
+
+	return 0;
+}
 
 static void omap_wdt_ping(struct omap_wdt_dev *wdev)
 {
@@ -145,13 +142,7 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;
 
-	if (cpu_is_omap16xx())
-		clk_enable(wdev->armwdt_ck);	/* Enable the clock */
-
-	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
-		clk_enable(wdev->mpu_wdt_ick);    /* Enable the interface clock */
-		clk_enable(wdev->mpu_wdt_fck);    /* Enable the functional clock */
-	}
+	omap_wdt_set_clock(wdev, 1);
 
 	/* initialize prescaler */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
@@ -177,16 +168,8 @@ static int omap_wdt_release(struct inode *inode, struct file *file)
 	 *      Shut off the timer unless NOWAYOUT is defined.
 	 */
 #ifndef CONFIG_WATCHDOG_NOWAYOUT
-
 	omap_wdt_disable(wdev);
-
-	if (cpu_is_omap16xx())
-		clk_disable(wdev->armwdt_ck);	/* Disable the clock */
-
-	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
-		clk_disable(wdev->mpu_wdt_ick);	/* Disable the clock */
-		clk_disable(wdev->mpu_wdt_fck);	/* Disable the clock */
-	}
+	omap_wdt_set_clock(wdev, 1);
 #else
 	printk(KERN_CRIT "omap_wdt: Unexpected close, not stopping!\n");
 #endif
@@ -232,12 +215,8 @@ omap_wdt_ioctl(struct inode *inode, struct file *file,
 	case WDIOC_GETSTATUS:
 		return put_user(0, (int __user *)arg);
 	case WDIOC_GETBOOTSTATUS:
-		if (cpu_is_omap16xx())
-			return put_user(__raw_readw(ARM_SYSST),
-					(int __user *)arg);
-		if (cpu_is_omap24xx())
-			return put_user(omap_prcm_get_reset_sources(),
-					(int __user *)arg);
+		return put_user(wdev->get_bootstatus(),
+				(int __user *)arg);
 	case WDIOC_KEEPALIVE:
 		omap_wdt_ping(wdev);
 		return 0;
@@ -269,6 +248,7 @@ static const struct file_operations omap_wdt_fops = {
 
 static int __init omap_wdt_probe(struct platform_device *pdev)
 {
+	struct omap_wdt_platform_data *pdata = pdev->dev.platform_data;
 	struct resource *res, *mem;
 	struct omap_wdt_dev *wdev;
 	int ret;
@@ -281,7 +261,7 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 	}
 
 	if (omap_wdt_dev) {
-		ret - EBUSY;
+		ret = EBUSY;
 		goto err_busy;
 	}
 
@@ -300,45 +280,30 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 
 	wdev->omap_wdt_users = 0;
 	wdev->mem = mem;
+	wdev->get_bootstatus = pdata->get_bootstatus;
+	wdev->set_clock = pdata->set_clock;
 
-	if (cpu_is_omap16xx()) {
-		wdev->armwdt_ck = clk_get(&pdev->dev, "armwdt_ck");
-		if (IS_ERR(wdev->armwdt_ck)) {
-			ret = PTR_ERR(wdev->armwdt_ck);
-			wdev->armwdt_ck = NULL;
-			goto err_clk;
-		}
+	/* omap1 has only the functional clock so we believe everybody
+	 * will pass that pointer correctly. For the interface clock,
+	 * we check whether that pointer is true to avoid null pointer
+	 * dereferences
+	 */
+	wdev->fck = clk_get(&pdev->dev, pdata->fck);
+	if (IS_ERR(wdev->fck)) {
+		ret = PTR_ERR(wdev->fck);
+		wdev->fck = NULL;
+		goto err_clk;
 	}
 
-	if (cpu_is_omap24xx()) {
-		wdev->mpu_wdt_ick = clk_get(&pdev->dev, "mpu_wdt_ick");
-		if (IS_ERR(wdev->mpu_wdt_ick)) {
-			ret = PTR_ERR(wdev->mpu_wdt_ick);
-			wdev->mpu_wdt_ick = NULL;
-			goto err_clk;
-		}
-		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "mpu_wdt_fck");
-		if (IS_ERR(wdev->mpu_wdt_fck)) {
-			ret = PTR_ERR(wdev->mpu_wdt_fck);
-			wdev->mpu_wdt_fck = NULL;
+	if (pdata->ick) {
+		wdev->ick = clk_get(&pdev->dev, pdata->ick);
+		if (IS_ERR(wdev->ick)) {
+			ret = PTR_ERR(wdev->ick);
+			wdev->ick = NULL;
 			goto err_clk;
 		}
 	}
 
-	if (cpu_is_omap34xx()) {
-		wdev->mpu_wdt_ick = clk_get(&pdev->dev, "wdt2_ick");
-		if (IS_ERR(wdev->mpu_wdt_ick)) {
-			ret = PTR_ERR(wdev->mpu_wdt_ick);
-			wdev->mpu_wdt_ick = NULL;
-			goto err_clk;
-		}
-		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "wdt2_fck");
-		if (IS_ERR(wdev->mpu_wdt_fck)) {
-			ret = PTR_ERR(wdev->mpu_wdt_fck);
-			wdev->mpu_wdt_fck = NULL;
-			goto err_clk;
-		}
-	}
 	wdev->base = ioremap(res->start, res->end - res->start + 1);
 	if (!wdev->base) {
 		ret = -ENOMEM;
@@ -378,12 +343,11 @@ err_ioremap:
 	wdev->base = NULL;
 
 err_clk:
-	if (wdev->armwdt_ck)
-		clk_put(wdev->armwdt_ck);
-	if (wdev->mpu_wdt_ick)
-		clk_put(wdev->mpu_wdt_ick);
-	if (wdev->mpu_wdt_fck)
-		clk_put(wdev->mpu_wdt_fck);
+	if (wdev->fck)
+		clk_put(wdev->fck);
+	if (wdev->ick)
+		clk_put(wdev->ick);
+
 	kfree(wdev);
 
 err_kzalloc:
@@ -411,19 +375,14 @@ static int omap_wdt_remove(struct platform_device *pdev)
 	release_resource(wdev->mem);
 	platform_set_drvdata(pdev, NULL);
 
-	if (wdev->armwdt_ck) {
-		clk_put(wdev->armwdt_ck);
-		wdev->armwdt_ck = NULL;
-	}
-
-	if (wdev->mpu_wdt_ick) {
-		clk_put(wdev->mpu_wdt_ick);
-		wdev->mpu_wdt_ick = NULL;
+	if (wdev->fck) {
+		clk_put(wdev->fck);
+		wdev->fck = NULL;
 	}
 
-	if (wdev->mpu_wdt_fck) {
-		clk_put(wdev->mpu_wdt_fck);
-		wdev->mpu_wdt_fck = NULL;
+	if (wdev->ick) {
+		clk_put(wdev->ick);
+		wdev->ick = NULL;
 	}
 
 	if (wdev->base)
diff --git a/include/linux/watchdog/omap_wdt.h b/include/linux/watchdog/omap_wdt.h
index 62e6a74..858d9a4 100644
--- a/include/linux/watchdog/omap_wdt.h
+++ b/include/linux/watchdog/omap_wdt.h
@@ -30,6 +30,9 @@
 #ifndef _OMAP_WATCHDOG_H
 #define _OMAP_WATCHDOG_H
 
+#include <linux/clk.h>
+#include <linux/miscdevice.h>
+
 #define OMAP_WATCHDOG_REV		(0x00)
 #define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
 #define OMAP_WATCHDOG_STATUS		(0x14)
@@ -51,4 +54,30 @@
 #define PTV			0	/* prescale */
 #define GET_WLDR_VAL(secs)	(0xffffffff - ((secs) * (32768/(1<<PTV))) + 1)
 
+struct omap_wdt_dev {
+	void __iomem    *base;          /* physical */
+	struct device   *dev;
+	int             omap_wdt_users;
+
+	struct clk      *ick;
+	struct clk      *fck;
+
+	struct resource *mem;
+	struct miscdevice omap_wdt_miscdev;
+
+	/* clock handling */
+	int	(*set_clock)(struct omap_wdt_dev *wdev, int status);
+
+	/* used for put_user() */
+	int	(*get_bootstatus)(void);
+};
+
+struct omap_wdt_platform_data {
+	const char	*ick;
+	const char	*fck;
+
+	int	(*set_clock)(struct omap_wdt_dev *wdev, int status);
+	int	(*get_bootstatus)(void);
+};
+
 #endif				/* _OMAP_WATCHDOG_H */
-- 
1.6.0.1.196.g01914

Re: [PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Russell King - ARM Linux]

On Fri, Sep 19, 2008 at 01:23:42AM +0300, Felipe Balbi wrote:
> +struct omap_wdt_platform_data {
> +	const char	*ick;
> +	const char	*fck;

Grumble.  The clk API is supposed to make this stuff completely
unnecessary - but OMAP doesn't implement the clk API correctly -
by using the 'name' as the sole key.  The 'name' argument is
supposed to be used to disambiguate between multiple clock
inputs on a single device. ;(

Re: [PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Felipe Balbi]

On Fri, Sep 19, 2008 at 12:20:06AM +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 19, 2008 at 01:23:42AM +0300, Felipe Balbi wrote:
> > +struct omap_wdt_platform_data {
> > +	const char	*ick;
> > +	const char	*fck;
> 
> Grumble.  The clk API is supposed to make this stuff completely
> unnecessary - but OMAP doesn't implement the clk API correctly -
> by using the 'name' as the sole key.  The 'name' argument is
> supposed to be used to disambiguate between multiple clock
> inputs on a single device. ;(

Yes, and we have two clocks for the same device_id on the bus. Right ?

-- 
balbi

Re: [PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Russell King - ARM Linux]

On Fri, Sep 19, 2008 at 02:23:25AM +0300, Felipe Balbi wrote:
> On Fri, Sep 19, 2008 at 12:20:06AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Sep 19, 2008 at 01:23:42AM +0300, Felipe Balbi wrote:
> > > +struct omap_wdt_platform_data {
> > > +	const char	*ick;
> > > +	const char	*fck;
> > 
> > Grumble.  The clk API is supposed to make this stuff completely
> > unnecessary - but OMAP doesn't implement the clk API correctly -
> > by using the 'name' as the sole key.  The 'name' argument is
> > supposed to be used to disambiguate between multiple clock
> > inputs on a single device. ;(
> 
> Yes, and we have two clocks for the same device_id on the bus. Right ?

That's not a problem.  I have an solution to solve this, but for the
time being, let's just keep the logic inside the omap_wdt driver.

Re: [PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Felipe Balbi]

On Fri, Sep 19, 2008 at 08:46:34AM +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 19, 2008 at 02:23:25AM +0300, Felipe Balbi wrote:
> > On Fri, Sep 19, 2008 at 12:20:06AM +0100, Russell King - ARM Linux wrote:
> > > On Fri, Sep 19, 2008 at 01:23:42AM +0300, Felipe Balbi wrote:
> > > > +struct omap_wdt_platform_data {
> > > > +	const char	*ick;
> > > > +	const char	*fck;
> > > 
> > > Grumble.  The clk API is supposed to make this stuff completely
> > > unnecessary - but OMAP doesn't implement the clk API correctly -
> > > by using the 'name' as the sole key.  The 'name' argument is
> > > supposed to be used to disambiguate between multiple clock
> > > inputs on a single device. ;(
> > 
> > Yes, and we have two clocks for the same device_id on the bus. Right ?
> 
> That's not a problem.  I have an solution to solve this, but for the
> time being, let's just keep the logic inside the omap_wdt driver.

Ok, I'll wait on this patch then. The sync with mainline and ioremap()
fix will be going to Wim soon, I just need to test for a while.

-- 
balbi

[PATCH 0/5] omap watchdog updaes

[Felipe Balbi]

Hi all,

Following are patches syncing, cleaning up and fixing omap watchdog
driver.

Patches 1-3 could be applied as is. Patches 4 and 5, on the other
hand, needs some discussion since I'd be creating a new directory
(include/linux/watchdog) and changing the clock handling.

Russel King told me, he'd like to hold on patch 5/5 as he'd have a
better solution for the clock handling soon. I'm sending that patch
away anyway so we can start the discussion on the mainling list about
that.

Felipe Balbi (5):
  watchdog: sync linux-omap changes
  watchdog: another ioremap() fix
  watchdog: cleanup a bit omap_wdt.c
  watchdog: move omap_wdt.h to include/linux/watchdog
  watchdog: introduce platform_data and remove cpu conditional code

 arch/arm/plat-omap/devices.c      |   76 ++++++++-
 drivers/watchdog/omap_wdt.c       |  333 ++++++++++++++++++++++---------------
 drivers/watchdog/omap_wdt.h       |   64 -------
 include/linux/watchdog/omap_wdt.h |   83 +++++++++
 4 files changed, 352 insertions(+), 204 deletions(-)
 delete mode 100644 drivers/watchdog/omap_wdt.h
 create mode 100644 include/linux/watchdog/omap_wdt.h

[PATCH 1/5] watchdog: sync linux-omap changes

[Felipe Balbi]

These are changes that have been sitting in linux-omap
and were never sent upstream.

Hopefully, it'll never happen again at least for this
driver.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 arch/arm/plat-omap/devices.c |   21 ++--
 drivers/watchdog/omap_wdt.c  |  287 +++++++++++++++++++++++++++---------------
 drivers/watchdog/omap_wdt.h  |   28 ++---
 3 files changed, 205 insertions(+), 131 deletions(-)

diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
index bc1cf30..f1eaa44 100644
--- a/arch/arm/plat-omap/devices.c
+++ b/arch/arm/plat-omap/devices.c
@@ -454,16 +454,8 @@ static inline void omap_init_uwire(void) {}
 
 #if	defined(CONFIG_OMAP_WATCHDOG) || defined(CONFIG_OMAP_WATCHDOG_MODULE)
 
-#ifdef CONFIG_ARCH_OMAP24XX
-#define	OMAP_WDT_BASE		0x48022000
-#else
-#define	OMAP_WDT_BASE		0xfffeb000
-#endif
-
 static struct resource wdt_resources[] = {
 	{
-		.start		= OMAP_WDT_BASE,
-		.end		= OMAP_WDT_BASE + 0x4f,
 		.flags		= IORESOURCE_MEM,
 	},
 };
@@ -477,6 +469,19 @@ static struct platform_device omap_wdt_device = {
 
 static void omap_init_wdt(void)
 {
+	if (cpu_is_omap16xx())
+		wdt_resources[0].start = 0xfffeb000;
+	else if (cpu_is_omap2420())
+		wdt_resources[0].start = 0x48022000; /* WDT2 */
+	else if (cpu_is_omap2430())
+		wdt_resources[0].start = 0x49016000; /* WDT2 */
+	else if (cpu_is_omap343x())
+		wdt_resources[0].start = 0x48314000; /* WDT2 */
+	else
+		return;
+
+	wdt_resources[0].end = wdt_resources[0].start + 0x4f;
+
 	(void) platform_device_register(&omap_wdt_device);
 }
 #else
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 3a11dad..e55f2cc 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -1,7 +1,7 @@
 /*
- * linux/drivers/char/watchdog/omap_wdt.c
+ * linux/drivers/watchdog/omap_wdt.c
  *
- * Watchdog driver for the TI OMAP 16xx & 24xx 32KHz (non-secure) watchdog
+ * Watchdog driver for the TI OMAP 16xx & 24xx/34xx 32KHz (non-secure) watchdog
  *
  * Author: MontaVista Software, Inc.
  *	 <gdavis@mvista.com> or <source@mvista.com>
@@ -39,6 +39,7 @@
 #include <linux/platform_device.h>
 #include <linux/moduleparam.h>
 #include <linux/clk.h>
+
 #include <linux/bitops.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
@@ -47,50 +48,63 @@
 
 #include "omap_wdt.h"
 
+static struct platform_device *omap_wdt_dev;
+
 static unsigned timer_margin;
 module_param(timer_margin, uint, 0);
 MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
 
-static int omap_wdt_users;
-static struct clk *armwdt_ck;
-static struct clk *mpu_wdt_ick;
-static struct clk *mpu_wdt_fck;
-
 static unsigned int wdt_trgr_pattern = 0x1234;
 static spinlock_t wdt_lock;
 
-static void omap_wdt_ping(void)
+struct omap_wdt_dev {
+	void __iomem    *base;          /* physical */
+	struct device   *dev;
+	int             omap_wdt_users;
+	struct clk      *armwdt_ck;
+	struct clk      *mpu_wdt_ick;
+	struct clk      *mpu_wdt_fck;
+	struct resource *mem;
+	struct miscdevice omap_wdt_miscdev;
+};
+
+static void omap_wdt_ping(struct omap_wdt_dev *wdev)
 {
+	void __iomem    *base = wdev->base;
 	/* wait for posted write to complete */
-	while ((omap_readl(OMAP_WATCHDOG_WPS)) & 0x08)
+	while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
 		cpu_relax();
 	wdt_trgr_pattern = ~wdt_trgr_pattern;
-	omap_writel(wdt_trgr_pattern, (OMAP_WATCHDOG_TGR));
+	omap_writel(wdt_trgr_pattern, (base + OMAP_WATCHDOG_TGR));
 	/* wait for posted write to complete */
-	while ((omap_readl(OMAP_WATCHDOG_WPS)) & 0x08)
+	while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
 		cpu_relax();
 	/* reloaded WCRR from WLDR */
 }
 
-static void omap_wdt_enable(void)
+static void omap_wdt_enable(struct omap_wdt_dev *wdev)
 {
+	void __iomem *base;
+	base = wdev->base;
 	/* Sequence to enable the watchdog */
-	omap_writel(0xBBBB, OMAP_WATCHDOG_SPR);
-	while ((omap_readl(OMAP_WATCHDOG_WPS)) & 0x10)
+	omap_writel(0xBBBB, base + OMAP_WATCHDOG_SPR);
+	while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
 		cpu_relax();
-	omap_writel(0x4444, OMAP_WATCHDOG_SPR);
-	while ((omap_readl(OMAP_WATCHDOG_WPS)) & 0x10)
+	omap_writel(0x4444, base + OMAP_WATCHDOG_SPR);
+	while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
 		cpu_relax();
 }
 
-static void omap_wdt_disable(void)
+static void omap_wdt_disable(struct omap_wdt_dev *wdev)
 {
+	void __iomem *base;
+	base = wdev->base;
 	/* sequence required to disable watchdog */
-	omap_writel(0xAAAA, OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
-	while (omap_readl(OMAP_WATCHDOG_WPS) & 0x10)
+	omap_writel(0xAAAA, base + OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
+	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
 		cpu_relax();
-	omap_writel(0x5555, OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
-	while (omap_readl(OMAP_WATCHDOG_WPS) & 0x10)
+	omap_writel(0x5555, base + OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
+	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
 		cpu_relax();
 }
 
@@ -103,15 +117,17 @@ static void omap_wdt_adjust_timeout(unsigned new_timeout)
 	timer_margin = new_timeout;
 }
 
-static void omap_wdt_set_timeout(void)
+static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
 {
 	u32 pre_margin = GET_WLDR_VAL(timer_margin);
+	void __iomem *base;
+	base = wdev->base;
 
 	/* just count up at 32 KHz */
-	while (omap_readl(OMAP_WATCHDOG_WPS) & 0x04)
+	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
-	omap_writel(pre_margin, OMAP_WATCHDOG_LDR);
-	while (omap_readl(OMAP_WATCHDOG_WPS) & 0x04)
+	omap_writel(pre_margin, base + OMAP_WATCHDOG_LDR);
+	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
 }
 
@@ -121,65 +137,69 @@ static void omap_wdt_set_timeout(void)
 
 static int omap_wdt_open(struct inode *inode, struct file *file)
 {
-	if (test_and_set_bit(1, (unsigned long *)&omap_wdt_users))
+	struct omap_wdt_dev *wdev;
+	void __iomem *base;
+	wdev = platform_get_drvdata(omap_wdt_dev);
+	base = wdev->base;
+	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;
 
 	if (cpu_is_omap16xx())
-		clk_enable(armwdt_ck);	/* Enable the clock */
+		clk_enable(wdev->armwdt_ck);	/* Enable the clock */
 
-	if (cpu_is_omap24xx()) {
-		clk_enable(mpu_wdt_ick);    /* Enable the interface clock */
-		clk_enable(mpu_wdt_fck);    /* Enable the functional clock */
+	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+		clk_enable(wdev->mpu_wdt_ick);    /* Enable the interface clock */
+		clk_enable(wdev->mpu_wdt_fck);    /* Enable the functional clock */
 	}
 
 	/* initialize prescaler */
-	while (omap_readl(OMAP_WATCHDOG_WPS) & 0x01)
+	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
 		cpu_relax();
-	omap_writel((1 << 5) | (PTV << 2), OMAP_WATCHDOG_CNTRL);
-	while (omap_readl(OMAP_WATCHDOG_WPS) & 0x01)
+	omap_writel((1 << 5) | (PTV << 2), base + OMAP_WATCHDOG_CNTRL);
+	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
 		cpu_relax();
 
-	omap_wdt_set_timeout();
-	omap_wdt_enable();
+	file->private_data = (void *) wdev;
+
+	omap_wdt_set_timeout(wdev);
+	omap_wdt_enable(wdev);
 	return nonseekable_open(inode, file);
 }
 
 static int omap_wdt_release(struct inode *inode, struct file *file)
 {
+	struct omap_wdt_dev *wdev;
+	wdev = file->private_data;
 	/*
 	 *      Shut off the timer unless NOWAYOUT is defined.
 	 */
 #ifndef CONFIG_WATCHDOG_NOWAYOUT
-	omap_wdt_disable();
 
-	if (cpu_is_omap16xx()) {
-		clk_disable(armwdt_ck);	/* Disable the clock */
-		clk_put(armwdt_ck);
-		armwdt_ck = NULL;
-	}
+	omap_wdt_disable(wdev);
 
-	if (cpu_is_omap24xx()) {
-		clk_disable(mpu_wdt_ick);	/* Disable the clock */
-		clk_disable(mpu_wdt_fck);	/* Disable the clock */
-		clk_put(mpu_wdt_ick);
-		clk_put(mpu_wdt_fck);
-		mpu_wdt_ick = NULL;
-		mpu_wdt_fck = NULL;
+	if (cpu_is_omap16xx())
+		clk_disable(wdev->armwdt_ck);	/* Disable the clock */
+
+	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+		clk_disable(wdev->mpu_wdt_ick);	/* Disable the clock */
+		clk_disable(wdev->mpu_wdt_fck);	/* Disable the clock */
 	}
 #else
 	printk(KERN_CRIT "omap_wdt: Unexpected close, not stopping!\n");
 #endif
-	omap_wdt_users = 0;
+	wdev->omap_wdt_users = 0;
 	return 0;
 }
 
 static ssize_t omap_wdt_write(struct file *file, const char __user *data,
 		size_t len, loff_t *ppos)
 {
+	struct omap_wdt_dev *wdev;
+	wdev = file->private_data;
 	/* Refresh LOAD_TIME. */
 	if (len) {
 		spin_lock(&wdt_lock);
-		omap_wdt_ping();
+		omap_wdt_ping(wdev);
 		spin_unlock(&wdt_lock);
 	}
 	return len;
@@ -188,12 +208,14 @@ static ssize_t omap_wdt_write(struct file *file, const char __user *data,
 static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 						unsigned long arg)
 {
+	struct omap_wdt_dev *wdev;
 	int new_margin;
 	static const struct watchdog_info ident = {
 		.identity = "OMAP Watchdog",
 		.options = WDIOF_SETTIMEOUT,
 		.firmware_version = 0,
 	};
+	wdev = file->private_data;
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
@@ -210,7 +232,7 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 					(int __user *)arg);
 	case WDIOC_KEEPALIVE:
 		spin_lock(&wdt_lock);
-		omap_wdt_ping();
+		omap_wdt_ping(wdev);
 		spin_unlock(&wdt_lock);
 		return 0;
 	case WDIOC_SETTIMEOUT:
@@ -218,19 +240,18 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 			return -EFAULT;
 		omap_wdt_adjust_timeout(new_margin);
 
-		spin_lock(&wdt_lock);
-		omap_wdt_disable();
-		omap_wdt_set_timeout();
-		omap_wdt_enable();
+		omap_wdt_disable(wdev);
+		omap_wdt_set_timeout(wdev);
+		omap_wdt_enable(wdev);
 
-		omap_wdt_ping();
-		spin_unlock(&wdt_lock);
+		omap_wdt_ping(wdev);
 		/* Fall */
 	case WDIOC_GETTIMEOUT:
 		return put_user(timer_margin, (int __user *)arg);
 	default:
 		return -ENOTTY;
 	}
+	return 0;
 }
 
 static const struct file_operations omap_wdt_fops = {
@@ -241,96 +262,150 @@ static const struct file_operations omap_wdt_fops = {
 	.release = omap_wdt_release,
 };
 
-static struct miscdevice omap_wdt_miscdev = {
-	.minor = WATCHDOG_MINOR,
-	.name = "watchdog",
-	.fops = &omap_wdt_fops,
-};
 
 static int __init omap_wdt_probe(struct platform_device *pdev)
 {
 	struct resource *res, *mem;
 	int ret;
+	struct omap_wdt_dev *wdev;
 
 	/* reserve static register mappings */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENOENT;
 
+	if (omap_wdt_dev)
+		return -EBUSY;
+
 	mem = request_mem_region(res->start, res->end - res->start + 1,
 				 pdev->name);
 	if (mem == NULL)
 		return -EBUSY;
 
-	platform_set_drvdata(pdev, mem);
-
-	omap_wdt_users = 0;
+	wdev = kzalloc(sizeof(struct omap_wdt_dev), GFP_KERNEL);
+	if (!wdev) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+	wdev->omap_wdt_users = 0;
+	wdev->mem = mem;
 
 	if (cpu_is_omap16xx()) {
-		armwdt_ck = clk_get(&pdev->dev, "armwdt_ck");
-		if (IS_ERR(armwdt_ck)) {
-			ret = PTR_ERR(armwdt_ck);
-			armwdt_ck = NULL;
+		wdev->armwdt_ck = clk_get(&pdev->dev, "armwdt_ck");
+		if (IS_ERR(wdev->armwdt_ck)) {
+			ret = PTR_ERR(wdev->armwdt_ck);
+			wdev->armwdt_ck = NULL;
 			goto fail;
 		}
 	}
 
 	if (cpu_is_omap24xx()) {
-		mpu_wdt_ick = clk_get(&pdev->dev, "mpu_wdt_ick");
-		if (IS_ERR(mpu_wdt_ick)) {
-			ret = PTR_ERR(mpu_wdt_ick);
-			mpu_wdt_ick = NULL;
+		wdev->mpu_wdt_ick = clk_get(&pdev->dev, "mpu_wdt_ick");
+		if (IS_ERR(wdev->mpu_wdt_ick)) {
+			ret = PTR_ERR(wdev->mpu_wdt_ick);
+			wdev->mpu_wdt_ick = NULL;
+			goto fail;
+		}
+		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "mpu_wdt_fck");
+		if (IS_ERR(wdev->mpu_wdt_fck)) {
+			ret = PTR_ERR(wdev->mpu_wdt_fck);
+			wdev->mpu_wdt_fck = NULL;
+			goto fail;
+		}
+	}
+
+	if (cpu_is_omap34xx()) {
+		wdev->mpu_wdt_ick = clk_get(&pdev->dev, "wdt2_ick");
+		if (IS_ERR(wdev->mpu_wdt_ick)) {
+			ret = PTR_ERR(wdev->mpu_wdt_ick);
+			wdev->mpu_wdt_ick = NULL;
 			goto fail;
 		}
-		mpu_wdt_fck = clk_get(&pdev->dev, "mpu_wdt_fck");
-		if (IS_ERR(mpu_wdt_fck)) {
-			ret = PTR_ERR(mpu_wdt_fck);
-			mpu_wdt_fck = NULL;
+		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "wdt2_fck");
+		if (IS_ERR(wdev->mpu_wdt_fck)) {
+			ret = PTR_ERR(wdev->mpu_wdt_fck);
+			wdev->mpu_wdt_fck = NULL;
 			goto fail;
 		}
 	}
+	wdev->base = (void __iomem *) (mem->start);
+	platform_set_drvdata(pdev, wdev);
 
-	omap_wdt_disable();
+	omap_wdt_disable(wdev);
 	omap_wdt_adjust_timeout(timer_margin);
 
-	omap_wdt_miscdev.parent = &pdev->dev;
-	ret = misc_register(&omap_wdt_miscdev);
+	wdev->omap_wdt_miscdev.parent = &pdev->dev;
+	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
+	wdev->omap_wdt_miscdev.name = "watchdog";
+	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
+
+	ret = misc_register(&(wdev->omap_wdt_miscdev));
 	if (ret)
 		goto fail;
 
-	pr_info("OMAP Watchdog Timer: initial timeout %d sec\n", timer_margin);
+	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec\n",
+		omap_readl(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
+		timer_margin);
 
 	/* autogate OCP interface clock */
-	omap_writel(0x01, OMAP_WATCHDOG_SYS_CONFIG);
+	omap_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);
+
+	omap_wdt_dev = pdev;
+
 	return 0;
 
 fail:
-	if (armwdt_ck)
-		clk_put(armwdt_ck);
-	if (mpu_wdt_ick)
-		clk_put(mpu_wdt_ick);
-	if (mpu_wdt_fck)
-		clk_put(mpu_wdt_fck);
-	release_resource(mem);
+	if (wdev) {
+		platform_set_drvdata(pdev, NULL);
+		if (wdev->armwdt_ck)
+			clk_put(wdev->armwdt_ck);
+		if (wdev->mpu_wdt_ick)
+			clk_put(wdev->mpu_wdt_ick);
+		if (wdev->mpu_wdt_fck)
+			clk_put(wdev->mpu_wdt_fck);
+		kfree(wdev);
+	}
+	if (mem) {
+		release_mem_region(res->start, res->end - res->start + 1);
+	}
 	return ret;
 }
 
 static void omap_wdt_shutdown(struct platform_device *pdev)
 {
-	omap_wdt_disable();
+	struct omap_wdt_dev *wdev;
+	wdev = platform_get_drvdata(pdev);
+
+	if (wdev->omap_wdt_users)
+		omap_wdt_disable(wdev);
 }
 
 static int omap_wdt_remove(struct platform_device *pdev)
 {
-	struct resource *mem = platform_get_drvdata(pdev);
-	misc_deregister(&omap_wdt_miscdev);
-	release_resource(mem);
-	if (armwdt_ck)
-		clk_put(armwdt_ck);
-	if (mpu_wdt_ick)
-		clk_put(mpu_wdt_ick);
-	if (mpu_wdt_fck)
-		clk_put(mpu_wdt_fck);
+	struct omap_wdt_dev *wdev;
+	wdev = platform_get_drvdata(pdev);
+	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (!res)
+		return -ENOENT;
+
+	misc_deregister(&(wdev->omap_wdt_miscdev));
+	release_mem_region(res->start, res->end - res->start + 1);
+	platform_set_drvdata(pdev, NULL);
+	if (wdev->armwdt_ck) {
+		clk_put(wdev->armwdt_ck);
+		wdev->armwdt_ck = NULL;
+	}
+	if (wdev->mpu_wdt_ick) {
+		clk_put(wdev->mpu_wdt_ick);
+		wdev->mpu_wdt_ick = NULL;
+	}
+	if (wdev->mpu_wdt_fck) {
+		clk_put(wdev->mpu_wdt_fck);
+		wdev->mpu_wdt_fck = NULL;
+	}
+	kfree(wdev);
+	omap_wdt_dev = NULL;
 	return 0;
 }
 
@@ -344,16 +419,20 @@ static int omap_wdt_remove(struct platform_device *pdev)
 
 static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
 {
-	if (omap_wdt_users)
-		omap_wdt_disable();
+	struct omap_wdt_dev *wdev;
+	wdev = platform_get_drvdata(pdev);
+	if (wdev->omap_wdt_users)
+		omap_wdt_disable(wdev);
 	return 0;
 }
 
 static int omap_wdt_resume(struct platform_device *pdev)
 {
-	if (omap_wdt_users) {
-		omap_wdt_enable();
-		omap_wdt_ping();
+	struct omap_wdt_dev *wdev;
+	wdev = platform_get_drvdata(pdev);
+	if (wdev->omap_wdt_users) {
+		omap_wdt_enable(wdev);
+		omap_wdt_ping(wdev);
 	}
 	return 0;
 }
diff --git a/drivers/watchdog/omap_wdt.h b/drivers/watchdog/omap_wdt.h
index 52a532a..fc02ec6 100644
--- a/drivers/watchdog/omap_wdt.h
+++ b/drivers/watchdog/omap_wdt.h
@@ -30,25 +30,15 @@
 #ifndef _OMAP_WATCHDOG_H
 #define _OMAP_WATCHDOG_H
 
-#define OMAP1610_WATCHDOG_BASE		0xfffeb000
-#define OMAP2420_WATCHDOG_BASE		0x48022000	/*WDT Timer 2 */
-
-#ifdef CONFIG_ARCH_OMAP24XX
-#define OMAP_WATCHDOG_BASE 		OMAP2420_WATCHDOG_BASE
-#else
-#define OMAP_WATCHDOG_BASE 		OMAP1610_WATCHDOG_BASE
-#define RM_RSTST_WKUP			0
-#endif
-
-#define OMAP_WATCHDOG_REV		(OMAP_WATCHDOG_BASE + 0x00)
-#define OMAP_WATCHDOG_SYS_CONFIG	(OMAP_WATCHDOG_BASE + 0x10)
-#define OMAP_WATCHDOG_STATUS		(OMAP_WATCHDOG_BASE + 0x14)
-#define OMAP_WATCHDOG_CNTRL		(OMAP_WATCHDOG_BASE + 0x24)
-#define OMAP_WATCHDOG_CRR		(OMAP_WATCHDOG_BASE + 0x28)
-#define OMAP_WATCHDOG_LDR		(OMAP_WATCHDOG_BASE + 0x2c)
-#define OMAP_WATCHDOG_TGR		(OMAP_WATCHDOG_BASE + 0x30)
-#define OMAP_WATCHDOG_WPS		(OMAP_WATCHDOG_BASE + 0x34)
-#define OMAP_WATCHDOG_SPR		(OMAP_WATCHDOG_BASE + 0x48)
+#define OMAP_WATCHDOG_REV		(0x00)
+#define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
+#define OMAP_WATCHDOG_STATUS		(0x14)
+#define OMAP_WATCHDOG_CNTRL		(0x24)
+#define OMAP_WATCHDOG_CRR		(0x28)
+#define OMAP_WATCHDOG_LDR		(0x2c)
+#define OMAP_WATCHDOG_TGR		(0x30)
+#define OMAP_WATCHDOG_WPS		(0x34)
+#define OMAP_WATCHDOG_SPR		(0x48)
 
 /* Using the prescaler, the OMAP watchdog could go for many
  * months before firing.  These limits work without scaling,
-- 
1.6.0.1.308.gede4c

[PATCH 2/5] watchdog: another ioremap() fix

[Felipe Balbi]

convert to use ioremap() and __raw_{read/write} friends.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
Signed-off-by: George G. Davis <gdavis@mvista.com>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 drivers/watchdog/omap_wdt.c |   50 +++++++++++++++++++++++++------------------
 1 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index e55f2cc..4efb87b 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -72,12 +72,12 @@ static void omap_wdt_ping(struct omap_wdt_dev *wdev)
 {
 	void __iomem    *base = wdev->base;
 	/* wait for posted write to complete */
-	while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
+	while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
 		cpu_relax();
 	wdt_trgr_pattern = ~wdt_trgr_pattern;
-	omap_writel(wdt_trgr_pattern, (base + OMAP_WATCHDOG_TGR));
+	__raw_writel(wdt_trgr_pattern, (base + OMAP_WATCHDOG_TGR));
 	/* wait for posted write to complete */
-	while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
+	while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
 		cpu_relax();
 	/* reloaded WCRR from WLDR */
 }
@@ -87,11 +87,11 @@ static void omap_wdt_enable(struct omap_wdt_dev *wdev)
 	void __iomem *base;
 	base = wdev->base;
 	/* Sequence to enable the watchdog */
-	omap_writel(0xBBBB, base + OMAP_WATCHDOG_SPR);
-	while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
+	__raw_writel(0xBBBB, base + OMAP_WATCHDOG_SPR);
+	while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
 		cpu_relax();
-	omap_writel(0x4444, base + OMAP_WATCHDOG_SPR);
-	while ((omap_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
+	__raw_writel(0x4444, base + OMAP_WATCHDOG_SPR);
+	while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
 		cpu_relax();
 }
 
@@ -100,11 +100,11 @@ static void omap_wdt_disable(struct omap_wdt_dev *wdev)
 	void __iomem *base;
 	base = wdev->base;
 	/* sequence required to disable watchdog */
-	omap_writel(0xAAAA, base + OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
-	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
+	__raw_writel(0xAAAA, base + OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
+	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
 		cpu_relax();
-	omap_writel(0x5555, base + OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
-	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
+	__raw_writel(0x5555, base + OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
+	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
 		cpu_relax();
 }
 
@@ -124,10 +124,10 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
 	base = wdev->base;
 
 	/* just count up at 32 KHz */
-	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
+	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
-	omap_writel(pre_margin, base + OMAP_WATCHDOG_LDR);
-	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
+	__raw_writel(pre_margin, base + OMAP_WATCHDOG_LDR);
+	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
 }
 
@@ -153,10 +153,10 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 	}
 
 	/* initialize prescaler */
-	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
+	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
 		cpu_relax();
-	omap_writel((1 << 5) | (PTV << 2), base + OMAP_WATCHDOG_CNTRL);
-	while (omap_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
+	__raw_writel((1 << 5) | (PTV << 2), base + OMAP_WATCHDOG_CNTRL);
+	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
 		cpu_relax();
 
 	file->private_data = (void *) wdev;
@@ -225,7 +225,7 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 		return put_user(0, (int __user *)arg);
 	case WDIOC_GETBOOTSTATUS:
 		if (cpu_is_omap16xx())
-			return put_user(omap_readw(ARM_SYSST),
+			return put_user(__raw_readw(ARM_SYSST),
 					(int __user *)arg);
 		if (cpu_is_omap24xx())
 			return put_user(omap_prcm_get_reset_sources(),
@@ -328,7 +328,12 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 			goto fail;
 		}
 	}
-	wdev->base = (void __iomem *) (mem->start);
+	wdev->base = ioremap(res->start, res->end - res->start + 1);
+	if (!wdev->base) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
 	platform_set_drvdata(pdev, wdev);
 
 	omap_wdt_disable(wdev);
@@ -344,11 +349,11 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 		goto fail;
 
 	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec\n",
-		omap_readl(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
+		__raw_readl(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
 		timer_margin);
 
 	/* autogate OCP interface clock */
-	omap_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);
+	__raw_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);
 
 	omap_wdt_dev = pdev;
 
@@ -363,6 +368,7 @@ fail:
 			clk_put(wdev->mpu_wdt_ick);
 		if (wdev->mpu_wdt_fck)
 			clk_put(wdev->mpu_wdt_fck);
+		iounmap(wdev->base);
 		kfree(wdev);
 	}
 	if (mem) {
@@ -404,6 +410,8 @@ static int omap_wdt_remove(struct platform_device *pdev)
 		clk_put(wdev->mpu_wdt_fck);
 		wdev->mpu_wdt_fck = NULL;
 	}
+	iounmap(wdev->base);
+
 	kfree(wdev);
 	omap_wdt_dev = NULL;
 	return 0;
-- 
1.6.0.1.308.gede4c

[PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[Felipe Balbi]

Trivial cleanup patch.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/watchdog/omap_wdt.c |  133 +++++++++++++++++++++++++-----------------
 1 files changed, 79 insertions(+), 54 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 4efb87b..b670ad5 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -71,11 +71,14 @@ struct omap_wdt_dev {
 static void omap_wdt_ping(struct omap_wdt_dev *wdev)
 {
 	void __iomem    *base = wdev->base;
+
 	/* wait for posted write to complete */
 	while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
 		cpu_relax();
+
 	wdt_trgr_pattern = ~wdt_trgr_pattern;
 	__raw_writel(wdt_trgr_pattern, (base + OMAP_WATCHDOG_TGR));
+
 	/* wait for posted write to complete */
 	while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x08)
 		cpu_relax();
@@ -84,12 +87,13 @@ static void omap_wdt_ping(struct omap_wdt_dev *wdev)
 
 static void omap_wdt_enable(struct omap_wdt_dev *wdev)
 {
-	void __iomem *base;
-	base = wdev->base;
+	void __iomem *base = wdev->base;
+
 	/* Sequence to enable the watchdog */
 	__raw_writel(0xBBBB, base + OMAP_WATCHDOG_SPR);
 	while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
 		cpu_relax();
+
 	__raw_writel(0x4444, base + OMAP_WATCHDOG_SPR);
 	while ((__raw_readl(base + OMAP_WATCHDOG_WPS)) & 0x10)
 		cpu_relax();
@@ -97,12 +101,13 @@ static void omap_wdt_enable(struct omap_wdt_dev *wdev)
 
 static void omap_wdt_disable(struct omap_wdt_dev *wdev)
 {
-	void __iomem *base;
-	base = wdev->base;
+	void __iomem *base = wdev->base;
+
 	/* sequence required to disable watchdog */
 	__raw_writel(0xAAAA, base + OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
 		cpu_relax();
+
 	__raw_writel(0x5555, base + OMAP_WATCHDOG_SPR);	/* TIMER_MODE */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x10)
 		cpu_relax();
@@ -120,12 +125,12 @@ static void omap_wdt_adjust_timeout(unsigned new_timeout)
 static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
 {
 	u32 pre_margin = GET_WLDR_VAL(timer_margin);
-	void __iomem *base;
-	base = wdev->base;
+	void __iomem *base = wdev->base;
 
 	/* just count up at 32 KHz */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
+
 	__raw_writel(pre_margin, base + OMAP_WATCHDOG_LDR);
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
@@ -134,13 +139,11 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
 /*
  *	Allow only one task to hold it open
  */
-
 static int omap_wdt_open(struct inode *inode, struct file *file)
 {
-	struct omap_wdt_dev *wdev;
-	void __iomem *base;
-	wdev = platform_get_drvdata(omap_wdt_dev);
-	base = wdev->base;
+	struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
+	void __iomem *base = wdev->base;
+
 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;
 
@@ -155,6 +158,7 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 	/* initialize prescaler */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
 		cpu_relax();
+
 	__raw_writel((1 << 5) | (PTV << 2), base + OMAP_WATCHDOG_CNTRL);
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
 		cpu_relax();
@@ -163,13 +167,14 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 
 	omap_wdt_set_timeout(wdev);
 	omap_wdt_enable(wdev);
+
 	return nonseekable_open(inode, file);
 }
 
 static int omap_wdt_release(struct inode *inode, struct file *file)
 {
-	struct omap_wdt_dev *wdev;
-	wdev = file->private_data;
+	struct omap_wdt_dev *wdev = file->private_data;
+
 	/*
 	 *      Shut off the timer unless NOWAYOUT is defined.
 	 */
@@ -188,14 +193,15 @@ static int omap_wdt_release(struct inode *inode, struct file *file)
 	printk(KERN_CRIT "omap_wdt: Unexpected close, not stopping!\n");
 #endif
 	wdev->omap_wdt_users = 0;
+
 	return 0;
 }
 
 static ssize_t omap_wdt_write(struct file *file, const char __user *data,
 		size_t len, loff_t *ppos)
 {
-	struct omap_wdt_dev *wdev;
-	wdev = file->private_data;
+	struct omap_wdt_dev *wdev = file->private_data;
+
 	/* Refresh LOAD_TIME. */
 	if (len) {
 		spin_lock(&wdt_lock);
@@ -215,6 +221,7 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 		.options = WDIOF_SETTIMEOUT,
 		.firmware_version = 0,
 	};
+
 	wdev = file->private_data;
 
 	switch (cmd) {
@@ -251,6 +258,7 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 	default:
 		return -ENOTTY;
 	}
+
 	return 0;
 }
 
@@ -262,31 +270,37 @@ static const struct file_operations omap_wdt_fops = {
 	.release = omap_wdt_release,
 };
 
-
 static int __init omap_wdt_probe(struct platform_device *pdev)
 {
 	struct resource *res, *mem;
-	int ret;
 	struct omap_wdt_dev *wdev;
+	int ret;
 
 	/* reserve static register mappings */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENOENT;
+	if (!res) {
+		ret = -ENOENT;
+		goto err_get_resource;
+	}
 
-	if (omap_wdt_dev)
-		return -EBUSY;
+	if (omap_wdt_dev) {
+		ret - EBUSY;
+		goto err_busy;
+	}
 
 	mem = request_mem_region(res->start, res->end - res->start + 1,
 				 pdev->name);
-	if (mem == NULL)
-		return -EBUSY;
+	if (!mem) {
+		ret = -EBUSY;
+		goto err_busy;
+	}
 
 	wdev = kzalloc(sizeof(struct omap_wdt_dev), GFP_KERNEL);
 	if (!wdev) {
 		ret = -ENOMEM;
-		goto fail;
+		goto err_kzalloc;
 	}
+
 	wdev->omap_wdt_users = 0;
 	wdev->mem = mem;
 
@@ -295,7 +309,7 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 		if (IS_ERR(wdev->armwdt_ck)) {
 			ret = PTR_ERR(wdev->armwdt_ck);
 			wdev->armwdt_ck = NULL;
-			goto fail;
+			goto err_clk;
 		}
 	}
 
@@ -304,13 +318,13 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 		if (IS_ERR(wdev->mpu_wdt_ick)) {
 			ret = PTR_ERR(wdev->mpu_wdt_ick);
 			wdev->mpu_wdt_ick = NULL;
-			goto fail;
+			goto err_clk;
 		}
 		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "mpu_wdt_fck");
 		if (IS_ERR(wdev->mpu_wdt_fck)) {
 			ret = PTR_ERR(wdev->mpu_wdt_fck);
 			wdev->mpu_wdt_fck = NULL;
-			goto fail;
+			goto err_clk;
 		}
 	}
 
@@ -319,19 +333,19 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 		if (IS_ERR(wdev->mpu_wdt_ick)) {
 			ret = PTR_ERR(wdev->mpu_wdt_ick);
 			wdev->mpu_wdt_ick = NULL;
-			goto fail;
+			goto err_clk;
 		}
 		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "wdt2_fck");
 		if (IS_ERR(wdev->mpu_wdt_fck)) {
 			ret = PTR_ERR(wdev->mpu_wdt_fck);
 			wdev->mpu_wdt_fck = NULL;
-			goto fail;
+			goto err_clk;
 		}
 	}
 	wdev->base = ioremap(res->start, res->end - res->start + 1);
 	if (!wdev->base) {
 		ret = -ENOMEM;
-		goto fail;
+		goto err_ioremap;
 	}
 
 	platform_set_drvdata(pdev, wdev);
@@ -346,7 +360,7 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 
 	ret = misc_register(&(wdev->omap_wdt_miscdev));
 	if (ret)
-		goto fail;
+		goto err_misc;
 
 	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec\n",
 		__raw_readl(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
@@ -359,28 +373,34 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 
 	return 0;
 
-fail:
-	if (wdev) {
-		platform_set_drvdata(pdev, NULL);
-		if (wdev->armwdt_ck)
-			clk_put(wdev->armwdt_ck);
-		if (wdev->mpu_wdt_ick)
-			clk_put(wdev->mpu_wdt_ick);
-		if (wdev->mpu_wdt_fck)
-			clk_put(wdev->mpu_wdt_fck);
-		iounmap(wdev->base);
-		kfree(wdev);
-	}
-	if (mem) {
-		release_mem_region(res->start, res->end - res->start + 1);
-	}
+err_misc:
+	platform_set_drvdata(pdev, NULL);
+	iounmap(wdev->base);
+
+err_ioremap:
+	wdev->base = NULL;
+
+err_clk:
+	if (wdev->armwdt_ck)
+		clk_put(wdev->armwdt_ck);
+	if (wdev->mpu_wdt_ick)
+		clk_put(wdev->mpu_wdt_ick);
+	if (wdev->mpu_wdt_fck)
+		clk_put(wdev->mpu_wdt_fck);
+	kfree(wdev);
+
+err_kzalloc:
+	release_mem_region(res->start, res->end - res->start + 1);
+
+err_busy:
+err_get_resource:
+
 	return ret;
 }
 
 static void omap_wdt_shutdown(struct platform_device *pdev)
 {
-	struct omap_wdt_dev *wdev;
-	wdev = platform_get_drvdata(pdev);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	if (wdev->omap_wdt_users)
 		omap_wdt_disable(wdev);
@@ -388,8 +408,7 @@ static void omap_wdt_shutdown(struct platform_device *pdev)
 
 static int omap_wdt_remove(struct platform_device *pdev)
 {
-	struct omap_wdt_dev *wdev;
-	wdev = platform_get_drvdata(pdev);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
 	if (!res)
@@ -398,14 +417,17 @@ static int omap_wdt_remove(struct platform_device *pdev)
 	misc_deregister(&(wdev->omap_wdt_miscdev));
 	release_mem_region(res->start, res->end - res->start + 1);
 	platform_set_drvdata(pdev, NULL);
+
 	if (wdev->armwdt_ck) {
 		clk_put(wdev->armwdt_ck);
 		wdev->armwdt_ck = NULL;
 	}
+
 	if (wdev->mpu_wdt_ick) {
 		clk_put(wdev->mpu_wdt_ick);
 		wdev->mpu_wdt_ick = NULL;
 	}
+
 	if (wdev->mpu_wdt_fck) {
 		clk_put(wdev->mpu_wdt_fck);
 		wdev->mpu_wdt_fck = NULL;
@@ -414,6 +436,7 @@ static int omap_wdt_remove(struct platform_device *pdev)
 
 	kfree(wdev);
 	omap_wdt_dev = NULL;
+
 	return 0;
 }
 
@@ -427,21 +450,23 @@ static int omap_wdt_remove(struct platform_device *pdev)
 
 static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
 {
-	struct omap_wdt_dev *wdev;
-	wdev = platform_get_drvdata(pdev);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
+
 	if (wdev->omap_wdt_users)
 		omap_wdt_disable(wdev);
+
 	return 0;
 }
 
 static int omap_wdt_resume(struct platform_device *pdev)
 {
-	struct omap_wdt_dev *wdev;
-	wdev = platform_get_drvdata(pdev);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
+
 	if (wdev->omap_wdt_users) {
 		omap_wdt_enable(wdev);
 		omap_wdt_ping(wdev);
 	}
+
 	return 0;
 }
 
-- 
1.6.0.1.308.gede4c

[PATCH 4/5] watchdog: move omap_wdt.h to include/linux/watchdog

[Felipe Balbi]

Create a new include/linux/watchdog directory
for holding watchdog chips headers, move omap_wdt.h
to the new location and update the include path in
the driver source.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/watchdog/omap_wdt.c       |    3 +-
 drivers/watchdog/omap_wdt.h       |   54 -------------------------------------
 include/linux/watchdog/omap_wdt.h |   54 +++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 56 deletions(-)
 delete mode 100644 drivers/watchdog/omap_wdt.h
 create mode 100644 include/linux/watchdog/omap_wdt.h

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index b670ad5..8b68bc0 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -33,6 +33,7 @@
 #include <linux/mm.h>
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>
+#include <linux/watchdog/omap_wdt.h>
 #include <linux/reboot.h>
 #include <linux/init.h>
 #include <linux/err.h>
@@ -46,8 +47,6 @@
 #include <mach/hardware.h>
 #include <mach/prcm.h>
 
-#include "omap_wdt.h"
-
 static struct platform_device *omap_wdt_dev;
 
 static unsigned timer_margin;
diff --git a/drivers/watchdog/omap_wdt.h b/drivers/watchdog/omap_wdt.h
deleted file mode 100644
index fc02ec6..0000000
--- a/drivers/watchdog/omap_wdt.h
+++ /dev/null
@@ -1,54 +0,0 @@
-/*
- *  linux/drivers/char/watchdog/omap_wdt.h
- *
- *  BRIEF MODULE DESCRIPTION
- *      OMAP Watchdog timer register definitions
- *
- *  Copyright (C) 2004 Texas Instruments.
- *
- *  This program is free software; you can redistribute  it and/or modify it
- *  under  the terms of  the GNU General  Public License as published by the
- *  Free Software Foundation;  either version 2 of the  License, or (at your
- *  option) any later version.
- *
- *  THIS  SOFTWARE  IS PROVIDED   ``AS  IS'' AND   ANY  EXPRESS OR IMPLIED
- *  WARRANTIES,   INCLUDING, BUT NOT  LIMITED  TO, THE IMPLIED WARRANTIES OF
- *  MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN
- *  NO  EVENT  SHALL   THE AUTHOR  BE    LIABLE FOR ANY   DIRECT, INDIRECT,
- *  INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
- *  NOT LIMITED   TO, PROCUREMENT OF  SUBSTITUTE GOODS  OR SERVICES; LOSS OF
- *  USE, DATA,  OR PROFITS; OR  BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
- *  ANY THEORY OF LIABILITY, WHETHER IN  CONTRACT, STRICT LIABILITY, OR TORT
- *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
- *  THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- *
- *  You should have received a copy of the  GNU General Public License along
- *  with this program; if not, write  to the Free Software Foundation, Inc.,
- *  675 Mass Ave, Cambridge, MA 02139, USA.
- */
-
-#ifndef _OMAP_WATCHDOG_H
-#define _OMAP_WATCHDOG_H
-
-#define OMAP_WATCHDOG_REV		(0x00)
-#define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
-#define OMAP_WATCHDOG_STATUS		(0x14)
-#define OMAP_WATCHDOG_CNTRL		(0x24)
-#define OMAP_WATCHDOG_CRR		(0x28)
-#define OMAP_WATCHDOG_LDR		(0x2c)
-#define OMAP_WATCHDOG_TGR		(0x30)
-#define OMAP_WATCHDOG_WPS		(0x34)
-#define OMAP_WATCHDOG_SPR		(0x48)
-
-/* Using the prescaler, the OMAP watchdog could go for many
- * months before firing.  These limits work without scaling,
- * with the 60 second default assumed by most tools and docs.
- */
-#define TIMER_MARGIN_MAX    	(24 * 60 * 60)	/* 1 day */
-#define TIMER_MARGIN_DEFAULT	60	/* 60 secs */
-#define TIMER_MARGIN_MIN	1
-
-#define PTV			0	/* prescale */
-#define GET_WLDR_VAL(secs)	(0xffffffff - ((secs) * (32768/(1<<PTV))) + 1)
-
-#endif				/* _OMAP_WATCHDOG_H */
diff --git a/include/linux/watchdog/omap_wdt.h b/include/linux/watchdog/omap_wdt.h
new file mode 100644
index 0000000..62e6a74
--- /dev/null
+++ b/include/linux/watchdog/omap_wdt.h
@@ -0,0 +1,54 @@
+/*
+ *  include/linux/watchdog/omap_wdt.h
+ *
+ *  BRIEF MODULE DESCRIPTION
+ *      OMAP Watchdog timer register definitions
+ *
+ *  Copyright (C) 2004 Texas Instruments.
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ *
+ *  THIS  SOFTWARE  IS PROVIDED   ``AS  IS'' AND   ANY  EXPRESS OR IMPLIED
+ *  WARRANTIES,   INCLUDING, BUT NOT  LIMITED  TO, THE IMPLIED WARRANTIES OF
+ *  MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN
+ *  NO  EVENT  SHALL   THE AUTHOR  BE    LIABLE FOR ANY   DIRECT, INDIRECT,
+ *  INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ *  NOT LIMITED   TO, PROCUREMENT OF  SUBSTITUTE GOODS  OR SERVICES; LOSS OF
+ *  USE, DATA,  OR PROFITS; OR  BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ *  ANY THEORY OF LIABILITY, WHETHER IN  CONTRACT, STRICT LIABILITY, OR TORT
+ *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ *  THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ *  You should have received a copy of the  GNU General Public License along
+ *  with this program; if not, write  to the Free Software Foundation, Inc.,
+ *  675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef _OMAP_WATCHDOG_H
+#define _OMAP_WATCHDOG_H
+
+#define OMAP_WATCHDOG_REV		(0x00)
+#define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
+#define OMAP_WATCHDOG_STATUS		(0x14)
+#define OMAP_WATCHDOG_CNTRL		(0x24)
+#define OMAP_WATCHDOG_CRR		(0x28)
+#define OMAP_WATCHDOG_LDR		(0x2c)
+#define OMAP_WATCHDOG_TGR		(0x30)
+#define OMAP_WATCHDOG_WPS		(0x34)
+#define OMAP_WATCHDOG_SPR		(0x48)
+
+/* Using the prescaler, the OMAP watchdog could go for many
+ * months before firing.  These limits work without scaling,
+ * with the 60 second default assumed by most tools and docs.
+ */
+#define TIMER_MARGIN_MAX	(24 * 60 * 60)	/* 1 day */
+#define TIMER_MARGIN_DEFAULT	60	/* 60 secs */
+#define TIMER_MARGIN_MIN	1
+
+#define PTV			0	/* prescale */
+#define GET_WLDR_VAL(secs)	(0xffffffff - ((secs) * (32768/(1<<PTV))) + 1)
+
+#endif				/* _OMAP_WATCHDOG_H */
-- 
1.6.0.1.308.gede4c

[PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Felipe Balbi]

Get rid of cpu conditional code.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 arch/arm/plat-omap/devices.c      |   65 +++++++++++++++++--
 drivers/watchdog/omap_wdt.c       |  126 ++++++++++++------------------------
 include/linux/watchdog/omap_wdt.h |   29 +++++++++
 3 files changed, 131 insertions(+), 89 deletions(-)

diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
index f1eaa44..d756743 100644
--- a/arch/arm/plat-omap/devices.c
+++ b/arch/arm/plat-omap/devices.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
+#include <linux/watchdog/omap_wdt.h>
 
 #include <mach/hardware.h>
 #include <asm/io.h>
@@ -460,6 +461,51 @@ static struct resource wdt_resources[] = {
 	},
 };
 
+static int omap_wdt_set_clock(struct omap_wdt_dev *wdev, int status)
+{
+	if (status) {
+		if (cpu_is_omap16xx())
+			clk_disable(wdev->fck);
+
+		if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+			clk_disable(wdev->ick);
+			clk_disable(wdev->fck);
+		}
+	} else {
+		if (cpu_is_omap16xx())
+			clk_disable(wdev->fck);
+
+		if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+			clk_disable(wdev->ick);
+			clk_disable(wdev->fck);
+		}
+	}
+
+	return 0;
+}
+
+static int omap_wdt_get_bootstatus(void)
+{
+	if (cpu_is_omap16xx())
+		return __raw_readw(ARM_SYSST);
+
+#if 0
+	/* REVISIT: When the patch introducing the following function
+	 * gets merged upstream, uncomment this code to enable omap2
+	 * support on omap watchdog driver.
+	 */
+	if (cpu_is_omap24xx())
+		return omap_prcm_get_reset_sources();
+#endif
+
+	return 0;
+}
+
+static struct omap_wdt_platform_data omap_wdt_pdata = {
+	.set_clock	= omap_wdt_set_clock,
+	.get_bootstatus	= omap_wdt_get_bootstatus,
+};
+
 static struct platform_device omap_wdt_device = {
 	.name	   = "omap_wdt",
 	.id	     = -1,
@@ -469,17 +515,26 @@ static struct platform_device omap_wdt_device = {
 
 static void omap_init_wdt(void)
 {
-	if (cpu_is_omap16xx())
+	if (cpu_is_omap16xx()) {
+		omap_wdt_pdata.fck = "armwdt_ck";
 		wdt_resources[0].start = 0xfffeb000;
-	else if (cpu_is_omap2420())
+	} else if (cpu_is_omap2420()) {
+		omap_wdt_pdata.fck = "mpu_wdt_ick";
+		omap_wdt_pdata.ick = "mpu_wdt_fck";
 		wdt_resources[0].start = 0x48022000; /* WDT2 */
-	else if (cpu_is_omap2430())
+	} else if (cpu_is_omap2430()) {
+		omap_wdt_pdata.fck = "mpu_wdt_ick";
+		omap_wdt_pdata.ick = "mpu_wdt_fck";
 		wdt_resources[0].start = 0x49016000; /* WDT2 */
-	else if (cpu_is_omap343x())
+	} else if (cpu_is_omap343x()) {
+		omap_wdt_pdata.fck = "wdt2_ick";
+		omap_wdt_pdata.ick = "wdt2_fck";
 		wdt_resources[0].start = 0x48314000; /* WDT2 */
-	else
+	} else {
 		return;
+	}
 
+	omap_wdt_device.dev.platform_data = &omap_wdt_pdata;
 	wdt_resources[0].end = wdt_resources[0].start + 0x4f;
 
 	(void) platform_device_register(&omap_wdt_device);
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 8b68bc0..c113953 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -47,25 +47,21 @@
 #include <mach/hardware.h>
 #include <mach/prcm.h>
 
-static struct platform_device *omap_wdt_dev;
-
 static unsigned timer_margin;
 module_param(timer_margin, uint, 0);
 MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
 
+static struct platform_device *omap_wdt_dev;
 static unsigned int wdt_trgr_pattern = 0x1234;
 static spinlock_t wdt_lock;
 
-struct omap_wdt_dev {
-	void __iomem    *base;          /* physical */
-	struct device   *dev;
-	int             omap_wdt_users;
-	struct clk      *armwdt_ck;
-	struct clk      *mpu_wdt_ick;
-	struct clk      *mpu_wdt_fck;
-	struct resource *mem;
-	struct miscdevice omap_wdt_miscdev;
-};
+static inline int omap_wdt_set_clock(struct omap_wdt_dev *wdev, int status)
+{
+	if (wdev->set_clock)
+		return wdev->set_clock(wdev, status);
+
+	return 0;
+}
 
 static void omap_wdt_ping(struct omap_wdt_dev *wdev)
 {
@@ -146,13 +142,7 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;
 
-	if (cpu_is_omap16xx())
-		clk_enable(wdev->armwdt_ck);	/* Enable the clock */
-
-	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
-		clk_enable(wdev->mpu_wdt_ick);    /* Enable the interface clock */
-		clk_enable(wdev->mpu_wdt_fck);    /* Enable the functional clock */
-	}
+	omap_wdt_set_clock(wdev, 1);
 
 	/* initialize prescaler */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
@@ -178,16 +168,8 @@ static int omap_wdt_release(struct inode *inode, struct file *file)
 	 *      Shut off the timer unless NOWAYOUT is defined.
 	 */
 #ifndef CONFIG_WATCHDOG_NOWAYOUT
-
 	omap_wdt_disable(wdev);
-
-	if (cpu_is_omap16xx())
-		clk_disable(wdev->armwdt_ck);	/* Disable the clock */
-
-	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
-		clk_disable(wdev->mpu_wdt_ick);	/* Disable the clock */
-		clk_disable(wdev->mpu_wdt_fck);	/* Disable the clock */
-	}
+	omap_wdt_set_clock(wdev, 1);
 #else
 	printk(KERN_CRIT "omap_wdt: Unexpected close, not stopping!\n");
 #endif
@@ -230,12 +212,8 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 	case WDIOC_GETSTATUS:
 		return put_user(0, (int __user *)arg);
 	case WDIOC_GETBOOTSTATUS:
-		if (cpu_is_omap16xx())
-			return put_user(__raw_readw(ARM_SYSST),
-					(int __user *)arg);
-		if (cpu_is_omap24xx())
-			return put_user(omap_prcm_get_reset_sources(),
-					(int __user *)arg);
+		return put_user(wdev->get_bootstatus(),
+				(int __user *)arg);
 	case WDIOC_KEEPALIVE:
 		spin_lock(&wdt_lock);
 		omap_wdt_ping(wdev);
@@ -271,6 +249,7 @@ static const struct file_operations omap_wdt_fops = {
 
 static int __init omap_wdt_probe(struct platform_device *pdev)
 {
+	struct omap_wdt_platform_data *pdata = pdev->dev.platform_data;
 	struct resource *res, *mem;
 	struct omap_wdt_dev *wdev;
 	int ret;
@@ -283,7 +262,7 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 	}
 
 	if (omap_wdt_dev) {
-		ret - EBUSY;
+		ret = EBUSY;
 		goto err_busy;
 	}
 
@@ -302,45 +281,30 @@ static int __init omap_wdt_probe(struct platform_device *pdev)
 
 	wdev->omap_wdt_users = 0;
 	wdev->mem = mem;
+	wdev->get_bootstatus = pdata->get_bootstatus;
+	wdev->set_clock = pdata->set_clock;
 
-	if (cpu_is_omap16xx()) {
-		wdev->armwdt_ck = clk_get(&pdev->dev, "armwdt_ck");
-		if (IS_ERR(wdev->armwdt_ck)) {
-			ret = PTR_ERR(wdev->armwdt_ck);
-			wdev->armwdt_ck = NULL;
-			goto err_clk;
-		}
+	/* omap1 has only the functional clock so we believe everybody
+	 * will pass that pointer correctly. For the interface clock,
+	 * we check whether that pointer is true to avoid null pointer
+	 * dereferences
+	 */
+	wdev->fck = clk_get(&pdev->dev, pdata->fck);
+	if (IS_ERR(wdev->fck)) {
+		ret = PTR_ERR(wdev->fck);
+		wdev->fck = NULL;
+		goto err_clk;
 	}
 
-	if (cpu_is_omap24xx()) {
-		wdev->mpu_wdt_ick = clk_get(&pdev->dev, "mpu_wdt_ick");
-		if (IS_ERR(wdev->mpu_wdt_ick)) {
-			ret = PTR_ERR(wdev->mpu_wdt_ick);
-			wdev->mpu_wdt_ick = NULL;
-			goto err_clk;
-		}
-		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "mpu_wdt_fck");
-		if (IS_ERR(wdev->mpu_wdt_fck)) {
-			ret = PTR_ERR(wdev->mpu_wdt_fck);
-			wdev->mpu_wdt_fck = NULL;
+	if (pdata->ick) {
+		wdev->ick = clk_get(&pdev->dev, pdata->ick);
+		if (IS_ERR(wdev->ick)) {
+			ret = PTR_ERR(wdev->ick);
+			wdev->ick = NULL;
 			goto err_clk;
 		}
 	}
 
-	if (cpu_is_omap34xx()) {
-		wdev->mpu_wdt_ick = clk_get(&pdev->dev, "wdt2_ick");
-		if (IS_ERR(wdev->mpu_wdt_ick)) {
-			ret = PTR_ERR(wdev->mpu_wdt_ick);
-			wdev->mpu_wdt_ick = NULL;
-			goto err_clk;
-		}
-		wdev->mpu_wdt_fck = clk_get(&pdev->dev, "wdt2_fck");
-		if (IS_ERR(wdev->mpu_wdt_fck)) {
-			ret = PTR_ERR(wdev->mpu_wdt_fck);
-			wdev->mpu_wdt_fck = NULL;
-			goto err_clk;
-		}
-	}
 	wdev->base = ioremap(res->start, res->end - res->start + 1);
 	if (!wdev->base) {
 		ret = -ENOMEM;
@@ -380,12 +344,11 @@ err_ioremap:
 	wdev->base = NULL;
 
 err_clk:
-	if (wdev->armwdt_ck)
-		clk_put(wdev->armwdt_ck);
-	if (wdev->mpu_wdt_ick)
-		clk_put(wdev->mpu_wdt_ick);
-	if (wdev->mpu_wdt_fck)
-		clk_put(wdev->mpu_wdt_fck);
+	if (wdev->fck)
+		clk_put(wdev->fck);
+	if (wdev->ick)
+		clk_put(wdev->ick);
+
 	kfree(wdev);
 
 err_kzalloc:
@@ -417,19 +380,14 @@ static int omap_wdt_remove(struct platform_device *pdev)
 	release_mem_region(res->start, res->end - res->start + 1);
 	platform_set_drvdata(pdev, NULL);
 
-	if (wdev->armwdt_ck) {
-		clk_put(wdev->armwdt_ck);
-		wdev->armwdt_ck = NULL;
-	}
-
-	if (wdev->mpu_wdt_ick) {
-		clk_put(wdev->mpu_wdt_ick);
-		wdev->mpu_wdt_ick = NULL;
+	if (wdev->fck) {
+		clk_put(wdev->fck);
+		wdev->fck = NULL;
 	}
 
-	if (wdev->mpu_wdt_fck) {
-		clk_put(wdev->mpu_wdt_fck);
-		wdev->mpu_wdt_fck = NULL;
+	if (wdev->ick) {
+		clk_put(wdev->ick);
+		wdev->ick = NULL;
 	}
 	iounmap(wdev->base);
 
diff --git a/include/linux/watchdog/omap_wdt.h b/include/linux/watchdog/omap_wdt.h
index 62e6a74..858d9a4 100644
--- a/include/linux/watchdog/omap_wdt.h
+++ b/include/linux/watchdog/omap_wdt.h
@@ -30,6 +30,9 @@
 #ifndef _OMAP_WATCHDOG_H
 #define _OMAP_WATCHDOG_H
 
+#include <linux/clk.h>
+#include <linux/miscdevice.h>
+
 #define OMAP_WATCHDOG_REV		(0x00)
 #define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
 #define OMAP_WATCHDOG_STATUS		(0x14)
@@ -51,4 +54,30 @@
 #define PTV			0	/* prescale */
 #define GET_WLDR_VAL(secs)	(0xffffffff - ((secs) * (32768/(1<<PTV))) + 1)
 
+struct omap_wdt_dev {
+	void __iomem    *base;          /* physical */
+	struct device   *dev;
+	int             omap_wdt_users;
+
+	struct clk      *ick;
+	struct clk      *fck;
+
+	struct resource *mem;
+	struct miscdevice omap_wdt_miscdev;
+
+	/* clock handling */
+	int	(*set_clock)(struct omap_wdt_dev *wdev, int status);
+
+	/* used for put_user() */
+	int	(*get_bootstatus)(void);
+};
+
+struct omap_wdt_platform_data {
+	const char	*ick;
+	const char	*fck;
+
+	int	(*set_clock)(struct omap_wdt_dev *wdev, int status);
+	int	(*get_bootstatus)(void);
+};
+
 #endif				/* _OMAP_WATCHDOG_H */
-- 
1.6.0.1.308.gede4c

Re: [PATCH 4/5] watchdog: move omap_wdt.h to include/linux/watchdog

[Alan Cox]

On Fri, 19 Sep 2008 13:32:38 +0300
Felipe Balbi <felipe.balbi@nokia.com> wrote:

> Create a new include/linux/watchdog directory
> for holding watchdog chips headers, move omap_wdt.h
> to the new location and update the include path in
> the driver source.

Headers that are driver private belong with the driver. In fact in many
cases they belong *in* the driver C file but that depends how much is
there.



> +#define OMAP_WATCHDOG_REV		(0x00)
> +#define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
> +#define OMAP_WATCHDOG_STATUS		(0x14)
> +#define OMAP_WATCHDOG_CNTRL		(0x24)
> +#define OMAP_WATCHDOG_CRR		(0x28)
> +#define OMAP_WATCHDOG_LDR		(0x2c)
> +#define OMAP_WATCHDOG_TGR		(0x30)
> +#define OMAP_WATCHDOG_WPS		(0x34)
> +#define OMAP_WATCHDOG_SPR		(0x48)
> +
> +/* Using the prescaler, the OMAP watchdog could go for many
> + * months before firing.  These limits work without scaling,
> + * with the 60 second default assumed by most tools and docs.
> + */
> +#define TIMER_MARGIN_MAX	(24 * 60 * 60)	/* 1 day */
> +#define TIMER_MARGIN_DEFAULT	60	/* 60 secs */
> +#define TIMER_MARGIN_MIN	1
> +
> +#define PTV			0	/* prescale */
> +#define GET_WLDR_VAL(secs)	(0xffffffff - ((secs) * (32768/(1<<PTV))) + 1)
> +
> +#endif				/* _OMAP_WATCHDOG_H */

You could just drop these into the C file given how small they are.

Alan

Re: [PATCH 4/5] watchdog: move omap_wdt.h to include/linux/watchdog

[Felipe Balbi]

On Fri, Sep 19, 2008 at 11:56:27AM +0100, ext Alan Cox wrote:
> On Fri, 19 Sep 2008 13:32:38 +0300
> Felipe Balbi <felipe.balbi@nokia.com> wrote:
> 
> > Create a new include/linux/watchdog directory
> > for holding watchdog chips headers, move omap_wdt.h
> > to the new location and update the include path in
> > the driver source.
> 
> Headers that are driver private belong with the driver. In fact in many
> cases they belong *in* the driver C file but that depends how much is
> there.
> 
> 
> 
> > +#define OMAP_WATCHDOG_REV		(0x00)
> > +#define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
> > +#define OMAP_WATCHDOG_STATUS		(0x14)
> > +#define OMAP_WATCHDOG_CNTRL		(0x24)
> > +#define OMAP_WATCHDOG_CRR		(0x28)
> > +#define OMAP_WATCHDOG_LDR		(0x2c)
> > +#define OMAP_WATCHDOG_TGR		(0x30)
> > +#define OMAP_WATCHDOG_WPS		(0x34)
> > +#define OMAP_WATCHDOG_SPR		(0x48)
> > +
> > +/* Using the prescaler, the OMAP watchdog could go for many
> > + * months before firing.  These limits work without scaling,
> > + * with the 60 second default assumed by most tools and docs.
> > + */
> > +#define TIMER_MARGIN_MAX	(24 * 60 * 60)	/* 1 day */
> > +#define TIMER_MARGIN_DEFAULT	60	/* 60 secs */
> > +#define TIMER_MARGIN_MIN	1
> > +
> > +#define PTV			0	/* prescale */
> > +#define GET_WLDR_VAL(secs)	(0xffffffff - ((secs) * (32768/(1<<PTV))) + 1)
> > +
> > +#endif				/* _OMAP_WATCHDOG_H */
> 
> You could just drop these into the C file given how small they are.

Sure, I'll update the patch. But if you look at patch 5/5, I'll need the
structures to define the set_clock() function. Should I create then
under <mach/omap_wdt.h> ??

-- 
balbi

Re: [PATCH 4/5] watchdog: move omap_wdt.h to include/linux/watchdog

[Alan Cox]

> Sure, I'll update the patch. But if you look at patch 5/5, I'll need the
> structures to define the set_clock() function. Should I create then
> under <mach/omap_wdt.h> ??

I think that would be better - you are not creating a public general
interface to watchdogs but a specific interface between the OMAP platform
code and the OMAP watchdog.

Alan

Re: [PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Russell King - ARM Linux]

On Fri, Sep 19, 2008 at 01:32:39PM +0300, Felipe Balbi wrote:
> @@ -469,17 +515,26 @@ static struct platform_device omap_wdt_device = {
>  
>  static void omap_init_wdt(void)
>  {
> -	if (cpu_is_omap16xx())
> +	if (cpu_is_omap16xx()) {
> +		omap_wdt_pdata.fck = "armwdt_ck";
>  		wdt_resources[0].start = 0xfffeb000;
> -	else if (cpu_is_omap2420())
> +	} else if (cpu_is_omap2420()) {
> +		omap_wdt_pdata.fck = "mpu_wdt_ick";
> +		omap_wdt_pdata.ick = "mpu_wdt_fck";

What happened to leaving this stuff inside omap_wdt.c as I said
during the previous review?  I really don't want to see such cleanups
when the real answer is to fix the OMAP clock API implementation.  It
just makes for more unnecessary noise when doing this, and then yet more
noise when we fix the OMAP clock API.

Please get rid of this and leave the clock naming crap inside omap_wdt.c.

Re: [PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Felipe Balbi]

On Fri, Sep 19, 2008 at 08:04:32PM +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 19, 2008 at 01:32:39PM +0300, Felipe Balbi wrote:
> > @@ -469,17 +515,26 @@ static struct platform_device omap_wdt_device = {
> >  
> >  static void omap_init_wdt(void)
> >  {
> > -	if (cpu_is_omap16xx())
> > +	if (cpu_is_omap16xx()) {
> > +		omap_wdt_pdata.fck = "armwdt_ck";
> >  		wdt_resources[0].start = 0xfffeb000;
> > -	else if (cpu_is_omap2420())
> > +	} else if (cpu_is_omap2420()) {
> > +		omap_wdt_pdata.fck = "mpu_wdt_ick";
> > +		omap_wdt_pdata.ick = "mpu_wdt_fck";
> 
> What happened to leaving this stuff inside omap_wdt.c as I said
> during the previous review?  I really don't want to see such cleanups
> when the real answer is to fix the OMAP clock API implementation.  It
> just makes for more unnecessary noise when doing this, and then yet more
> noise when we fix the OMAP clock API.
> 
> Please get rid of this and leave the clock naming crap inside omap_wdt.c.

Well, patches 4 and 5 should be ignored. Should I resend or could I rely
on the fact that people won't pick them up ?

-- 
balbi

Re: [PATCH 1/5] watchdog: sync linux-omap changes

[Russell King - ARM Linux]

You're getting there with this patch, but still not completely up to
snuff.

On Fri, Sep 19, 2008 at 01:32:35PM +0300, Felipe Balbi wrote:
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 3a11dad..e55f2cc 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -39,6 +39,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/moduleparam.h>
>  #include <linux/clk.h>
> +
>  #include <linux/bitops.h>
>  #include <linux/io.h>
>  #include <linux/uaccess.h>

This hunk is unnecessary.

> @@ -218,19 +240,18 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
>  			return -EFAULT;
>  		omap_wdt_adjust_timeout(new_margin);
>  
> -		spin_lock(&wdt_lock);
> -		omap_wdt_disable();
> -		omap_wdt_set_timeout();
> -		omap_wdt_enable();
> +		omap_wdt_disable(wdev);
> +		omap_wdt_set_timeout(wdev);
> +		omap_wdt_enable(wdev);
>  
> -		omap_wdt_ping();
> -		spin_unlock(&wdt_lock);
> +		omap_wdt_ping(wdev);

This is removing the spin lock which should remain.

>  		/* Fall */
>  	case WDIOC_GETTIMEOUT:
>  		return put_user(timer_margin, (int __user *)arg);
>  	default:
>  		return -ENOTTY;
>  	}
> +	return 0;

And this return statement shouldn't be required.

Apart from those three points, nice work.

Re: [PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Russell King - ARM Linux]

On Sat, Sep 20, 2008 at 12:33:34AM +0300, Felipe Balbi wrote:
> Well, patches 4 and 5 should be ignored. Should I resend or could I rely
> on the fact that people won't pick them up ?

Given my comments on patch 1, it's probably a good idea to resend
just 1 to 3.  We can then talk about 4 and 5 some more.

Wim - can you let me know when you merge the followup patches into your
tree please?

Re: [PATCH 1/5] watchdog: sync linux-omap changes

[David Brownell]

On Friday 19 September 2008, Felipe Balbi wrote:
> - * linux/drivers/char/watchdog/omap_wdt.c
> + * linux/drivers/watchdog/omap_wdt.c

current style omits the paths (just use "omap_wdt.c")
since they change periodically...

Re: [PATCH 1/5] watchdog: sync linux-omap changes

[David Brownell]

On Friday 19 September 2008, Felipe Balbi wrote:
> +static struct platform_device *omap_wdt_dev;
> +
>  ...
> +struct omap_wdt_dev {
> +       void __iomem    *base;          /* physical */
> +       struct device   *dev;


You don't need both omap_wdt_dev (platform device)
and omap_wdt_dev.dev (hmm, never used).  In fact
the former isn't needed either ... its role seems
to be ensure only one watchdog device gets bound,
which is more naturally done by not registering
more than one such platform device.
 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[David Brownell]

On Friday 19 September 2008, Felipe Balbi wrote:
>  static int omap_wdt_open(struct inode *inode, struct file *file)
>  {
> -       struct omap_wdt_dev *wdev;
> -       void __iomem *base;
> -       wdev = platform_get_drvdata(omap_wdt_dev);
> -       base = wdev->base;
> +       struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
> +       void __iomem *base = wdev->base;
> +

Oh, I see where "omap_wdt_dev" (global) gets used.  The normal
way to do stuff like that is using void* pointers placed in the
inode and file structures for exactly that purpose.


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

Re: [PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Wim Van Sebroeck]

Hi Balbi,

> Well, patches 4 and 5 should be ignored. Should I resend or could I rely
> on the fact that people won't pick them up ?

I will not pick them up (to add them in the watchdog tree).

Kind regards,
Wim.

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[Russell King - ARM Linux]

On Fri, Sep 19, 2008 at 05:41:44PM -0700, David Brownell wrote:
> On Friday 19 September 2008, Felipe Balbi wrote:
> >  static int omap_wdt_open(struct inode *inode, struct file *file)
> >  {
> > -       struct omap_wdt_dev *wdev;
> > -       void __iomem *base;
> > -       wdev = platform_get_drvdata(omap_wdt_dev);
> > -       base = wdev->base;
> > +       struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
> > +       void __iomem *base = wdev->base;
> > +
> 
> Oh, I see where "omap_wdt_dev" (global) gets used.  The normal
> way to do stuff like that is using void* pointers placed in the
> inode and file structures for exactly that purpose.

You don't have an inode or a file structure until open() is called -
at which point it _is_ placed in file->private_data.  So this driver
is doing the right thing.

Re: [PATCH 5/5] watchdog: introduce platform_data and remove cpu conditional code

[Wim Van Sebroeck]

Hi Russell,

> Given my comments on patch 1, it's probably a good idea to resend
> just 1 to 3.  We can then talk about 4 and 5 some more.
> 
> Wim - can you let me know when you merge the followup patches into your
> tree please?

Will add patches 1 to 3 when everyone is OK with them. I still saw
some comments from David.

Kind Regards,
Wim.

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[David Brownell]

On Saturday 20 September 2008, Russell King - ARM Linux wrote:
> On Fri, Sep 19, 2008 at 05:41:44PM -0700, David Brownell wrote:
> > On Friday 19 September 2008, Felipe Balbi wrote:
> > >  static int omap_wdt_open(struct inode *inode, struct file *file)
> > >  {
> > > -       struct omap_wdt_dev *wdev;
> > > -       void __iomem *base;
> > > -       wdev = platform_get_drvdata(omap_wdt_dev);
> > > -       base = wdev->base;
> > > +       struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
> > > +       void __iomem *base = wdev->base;
> > > +
> > 
> > Oh, I see where "omap_wdt_dev" (global) gets used.  The normal
> > way to do stuff like that is using void* pointers placed in the
> > inode and file structures for exactly that purpose.
> 
> You don't have an inode or a file structure until open() is called -
> at which point it _is_ placed in file->private_data.  So this driver
> is doing the right thing.

Well, the conventional thing for misc drivers, at any rate.  In
various other drivers, inode->i_private is set up earlier, just
to avoid such a need for globals (or equivalent).

One could argue that this idiom is ugly ... and fix it by having
misc_open() in drivers/char/misc.c initialize i_private before
delegating to the miscdevice->fops->open().  Even just setting
it to the miscdevice pointer would suffice with this driver;
container_of(i_private, struct omap_wdt_dev, omap_wdt_miscdev)
would then return what get_drvdata() returns, sans global.

But that wouldn't be just cleaning up this watchdog.

= Dave


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

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[Russell King - ARM Linux]

On Sat, Sep 20, 2008 at 08:32:14AM -0700, David Brownell wrote:
> On Saturday 20 September 2008, Russell King - ARM Linux wrote:
> > On Fri, Sep 19, 2008 at 05:41:44PM -0700, David Brownell wrote:
> > > On Friday 19 September 2008, Felipe Balbi wrote:
> > > >  static int omap_wdt_open(struct inode *inode, struct file *file)
> > > >  {
> > > > -       struct omap_wdt_dev *wdev;
> > > > -       void __iomem *base;
> > > > -       wdev = platform_get_drvdata(omap_wdt_dev);
> > > > -       base = wdev->base;
> > > > +       struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
> > > > +       void __iomem *base = wdev->base;
> > > > +
> > > 
> > > Oh, I see where "omap_wdt_dev" (global) gets used.  The normal
> > > way to do stuff like that is using void* pointers placed in the
> > > inode and file structures for exactly that purpose.
> > 
> > You don't have an inode or a file structure until open() is called -
> > at which point it _is_ placed in file->private_data.  So this driver
> > is doing the right thing.
> 
> Well, the conventional thing for misc drivers, at any rate.  In
> various other drivers, inode->i_private is set up earlier, just
> to avoid such a need for globals (or equivalent).

None that I know about - generally other drivers look up their private
data in some kind of array and assign to file->private_data in their
open() method - in much the same way that watchdog and misc drivers do.

See __ptmx_open, __tty_open, pp_open, apm_open, hpet_open, mbcs_open,
raw_open, etc.

If you look at data structure lifetimes, the lifetime of 'file' is for
the duration that any one particular instance of the file is open, and
when closed, it's destroyed.  It is not shared between separate opens
of the same node.

The 'inode' is shared between separate opens, and can be discarded when
the node is not open by anyone - in other words, it's not persistent.
So the only time that an inode structure is guaranteed to be present
is just before the node is opened - useless for passing private
pointers from the registration function through to the open() function.

So, you need to store the private data pointer somewhere no matter what.

The simplest solution is as the watchdog drivers are doing.  You can
only have one watchdog driver anyway, so there's no problem having a
single static global device pointer to allow you to carry your private
data across to the open() function.

And anyway, the point of these patches is not to fix issues like this.
It's to get what's in mainline updated to what's in the OMAP tree so
stuff can move forwards.  So, let's not go down rabbit warrens trying
to find obscure new issues which lots of other code already "suffers"
from.

We're into the third day on this one driver.  If every OMAP driver takes
this long, we're still going to be struggling with this beyond Christmas,
probably no further forward since other stuff will have regressed, and
we'll have to start over again.

Yes, make sure what we're submitting is correct.  But don't introduce any
_new_ unnecessary changes while we're trying to merge stuff upstream.  Do
that only once it's upstream and send those changes upstream.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[Alan Cox]

> One could argue that this idiom is ugly ... and fix it by having
> misc_open() in drivers/char/misc.c initialize i_private before
> delegating to the miscdevice->fops->open().  Even just setting
> it to the miscdevice pointer would suffice with this driver;
> container_of(i_private, struct omap_wdt_dev, omap_wdt_miscdev)
> would then return what get_drvdata() returns, sans global.
> 
> But that wouldn't be just cleaning up this watchdog.

There are a couple of takes are reworking all the watchdog core code to
get rid of that stuff - so its probably not worth worrying about as
pretty soon I'd hope watchdogs are mostly using one of the new interfaces
and taking struct watchdog for ops (and in the case of the watchdog core
code I proposed don't even usually need open methods at all)

Alan
--
#include <stdsig.h>

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[David Brownell]

> > > > Oh, I see where "omap_wdt_dev" (global) gets used.  The normal
> > > > way to do stuff like that is using void* pointers placed in the
> > > > inode and file structures for exactly that purpose.
> > > 
> > > You don't have an inode or a file structure until open() is called -
> > > at which point it _is_ placed in file->private_data.  So this driver
> > > is doing the right thing.
> > 
> > Well, the conventional thing for misc drivers, at any rate.  In
> > various other drivers, inode->i_private is set up earlier, just
> > to avoid such a need for globals (or equivalent).
> 
> None that I know about - generally other drivers look up their private
> data in some kind of array and assign to file->private_data in their
> open() method - in much the same way that watchdog and misc drivers do.

Yet i_private gets set up *somewhere* else that field wouldn't exist.
Look around; you'll see it gets used.  (My quick grep started in the
USB tree, where it turns out both usbfs and gadgetfs use it.)

When the inode stays permanently in memory that's simple to manage.
Regardless, for misc drivers the scheme I mentioned is equally simple:

--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -147,6 +147,7 @@ static int misc_open(struct inode * inode, struct file * file)
 	err = 0;
 	old_fops = file->f_op;
 	file->f_op = new_fops;
+	inode->i_private = c;
 	if (file->f_op->open) {
 		err=file->f_op->open(inode,file);
 		if (err) {

> The simplest solution is as the watchdog drivers are doing.

I'll disagree.  It requires extra state even in this simple case;
state of a generically undesirable flavor (global).  And likewise,
it requires lookup mechanisms ... which if you look around, tend
to be rather error prone, locking often gets goofed up there.

Familiar and simple != simplest, != best.  It will often become
cargo cult programming.


> And anyway, the point of these patches is not to fix issues like this.
> It's to get what's in mainline updated to what's in the OMAP tree so
> stuff can move forwards.  So, let's not go down rabbit warrens trying
> to find obscure new issues which lots of other code already "suffers"
> from.

Right, my original comment pointed out one thing that was clearly
wrong (extra/unused struct field) and one odd thing (the global).

You said "not that odd, here's why"; I said "hmm, well OK but it's
still got problem X, which isn't for fixing in this patchset".

(Leaving the extra/unused struct field still an issue.  It came
from the original d99241c86f6ccd1226935f8f59d3bb17a4186b85 patch
which made the OMAP tree diverge from upstream.  I suspect that
one didn't get much review, partly because at that time OMAP3 was
much less available than now.  Pushing that upstream may not even
have been an option then.)


> We're into the third day on this one driver.  If every OMAP driver takes
> this long ...

That seems like a strange way to account things.  It presumes
that the only time review comments should be accepted is within
a day of the patch getting posted.  Regardless of whether the
reviewer has time at that point.

If you *really* wanted to avoid wasting time you wouldn't have
replied to my previous post, which agreed that one point I raised
was a non-issue for this particular patch series.

- Dave

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[Russell King - ARM Linux]

On Sat, Sep 20, 2008 at 10:18:41AM -0700, David Brownell wrote:
> > None that I know about - generally other drivers look up their private
> > data in some kind of array and assign to file->private_data in their
> > open() method - in much the same way that watchdog and misc drivers do.
> 
> Yet i_private gets set up *somewhere* else that field wouldn't exist.
> Look around; you'll see it gets used.  (My quick grep started in the
> USB tree, where it turns out both usbfs and gadgetfs use it.)

Both of which are filesystems which have more control over the lifetime
of the inode.  Device drivers don't have such luxuries.

> > The simplest solution is as the watchdog drivers are doing.
> 
> I'll disagree.  It requires extra state even in this simple case;
> state of a generically undesirable flavor (global).  And likewise,
> it requires lookup mechanisms ... which if you look around, tend
> to be rather error prone, locking often gets goofed up there.
> 
> Familiar and simple != simplest, != best.  It will often become
> cargo cult programming.

Well, are you going to manufacture a patch to update all the watchdog
drivers to use your new i_private method, and get that merged into
Wim's tree now, so that then the omap watchdog drivers can satisfy
your apparant objection (which Wim _has_ taken as an objection against
them going in)?

> > And anyway, the point of these patches is not to fix issues like this.
> > It's to get what's in mainline updated to what's in the OMAP tree so
> > stuff can move forwards.  So, let's not go down rabbit warrens trying
> > to find obscure new issues which lots of other code already "suffers"
> > from.
> 
> Right, my original comment pointed out one thing that was clearly
> wrong (extra/unused struct field) and one odd thing (the global).
> 
> You said "not that odd, here's why"; I said "hmm, well OK but it's
> still got problem X, which isn't for fixing in this patchset".

The "well OK" didn't come over at all - neither I nor Wim seem to have
received that point.

> > We're into the third day on this one driver.  If every OMAP driver takes
> > this long ...
> 
> That seems like a strange way to account things.  It presumes
> that the only time review comments should be accepted is within
> a day of the patch getting posted.  Regardless of whether the
> reviewer has time at that point.

You define accounting for things in real time as "strange" - lol.  Your
following sentences don't follow either.

My point is that we currently have a BIG problem, and that is the OMAP
fork being so far out of line with mainline, it isn't funny.  It's
causing lots of pain for everyone here.  Folk are screaming for mainline
to be buildable for OMAP.

There are two approaches to achieve that: take each driver, polish it
for weeks on end until it's nice and shiney, and then submit it upstream.
Eventually, given enough man hours, you'll get to the point where you've
pushed everything upstream, but in the mean time, new work has been
queued so you need to start at the beginning again.  You've got a job
for life constantly polishing code.

The other approach is to decide that we have what we have, and that in
the interests of efficiently reducing divergence, merging the upstream
changes with the downstream changes and pushing the result upstream ASAP.
Once merged, further improvements and cleanups can be made by pushing
them separately upstream along with any other bug fixes.

Given the amount of divergence, the only approach which gives realistic
progress is the second one.

If you think the first approach is the way to go, then please join in
with Tony and myself reviewing the _entire_ OMAP tree, polishing every
patch, and pushing it upstream.  And I mean _everything_.  Not just the
USB stuff.  Encourage everyone else to do the same - because it will
take an army of individuals to make any forward progress.

> If you *really* wanted to avoid wasting time you wouldn't have
> replied to my previous post, which agreed that one point I raised
> was a non-issue for this particular patch series.

Wim said: "Will add patches 1 to 3 when everyone is OK with them. I
still saw some comments from David."

So, would you like to clearly tell Wim what your comments mean as far
as merging this patch series?  It seems I'm not the only one who's
confused as to the intention and meaning of your comments.

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [080920 11:01]:
> On Sat, Sep 20, 2008 at 10:18:41AM -0700, David Brownell wrote:

<snip snip>

> 
> > > We're into the third day on this one driver.  If every OMAP driver takes
> > > this long ...
> > 
> > That seems like a strange way to account things.  It presumes
> > that the only time review comments should be accepted is within
> > a day of the patch getting posted.  Regardless of whether the
> > reviewer has time at that point.
> 
> You define accounting for things in real time as "strange" - lol.  Your
> following sentences don't follow either.
> 
> My point is that we currently have a BIG problem, and that is the OMAP
> fork being so far out of line with mainline, it isn't funny.  It's
> causing lots of pain for everyone here.  Folk are screaming for mainline
> to be buildable for OMAP.
> 
> There are two approaches to achieve that: take each driver, polish it
> for weeks on end until it's nice and shiney, and then submit it upstream.
> Eventually, given enough man hours, you'll get to the point where you've
> pushed everything upstream, but in the mean time, new work has been
> queued so you need to start at the beginning again.  You've got a job
> for life constantly polishing code.
> 
> The other approach is to decide that we have what we have, and that in
> the interests of efficiently reducing divergence, merging the upstream
> changes with the downstream changes and pushing the result upstream ASAP.
> Once merged, further improvements and cleanups can be made by pushing
> them separately upstream along with any other bug fixes.
> 
> Given the amount of divergence, the only approach which gives realistic
> progress is the second one.
> 
> If you think the first approach is the way to go, then please join in
> with Tony and myself reviewing the _entire_ OMAP tree, polishing every
> patch, and pushing it upstream.  And I mean _everything_.  Not just the
> USB stuff.  Encourage everyone else to do the same - because it will
> take an army of individuals to make any forward progress.

Hey, you gotta give Dave some credit! Dave's been polishing tons of omap
code in addition to the USB, at least I2C, gpio, SPI come to mind. Not to
mention all the blinking leds!

Regarding getting and army of people to fix code, we need to start
following another standard policy: All drivers must have a MAINTAINER
who is capable of fixing things, and ideally doing things the right
way from the start.

It's not going to be enough that few people try to fix stuff and get
burnt out on it over and over again when new omaps come around every
1.5 years.

<snip snip>

Tony

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[David Brownell]

On Saturday 20 September 2008, Russell King - ARM Linux wrote:
> On Sat, Sep 20, 2008 at 10:18:41AM -0700, David Brownell wrote:
>
> > Yet i_private gets set up *somewhere* else that field wouldn't exist.
> > Look around; you'll see it gets used.  (My quick grep started in the
> > USB tree, where it turns out both usbfs and gadgetfs use it.)
> 
> Both of which are filesystems which have more control over the lifetime
> of the inode.  Device drivers don't have such luxuries.

Both of them are the *kernel part* of a *user mode device driver* ...
but regardless, that doesn't invalidate what I said (and showed!)
about how easy it is to initialize that (even in this watchdog case).


> Well, are you going to manufacture a patch to update all the watchdog
> drivers to use your new i_private method,

If Alan Cox hadn't reminded us about pending cleanups to create more
of a real watchdog framework, I might well have done so.  The core
update was trivial; driver updates could trickle in when convenient.
No "flag day" needed.  But as I noted:  Not part of this patch series.


> > > We're into the third day on this one driver.  If every OMAP driver
> > > takes this long ...
> > 
> > That seems like a strange way to account things.  It  presumes
> > that the only time review comments should be accepted is within
> > a day of the patch getting posted.  Regardless of whether the
> > reviewer has time at that point.
> 
> You define accounting for things in real time as "strange" - lol.  Your
> following sentences don't follow either.

Redefining my words doesn't help, unless you're intent on creating
arguments instead of consensus.

Review rarely happens all at once, unless very few people look at
the code.  Discouraging review is *extremely* strange.  Most developers
want loads more than they ever get.  Yet you complained about me
not reviewing at the same time you did (I happened to be traveling)
despite there being no rush to queue this.  Also strange.


> My point is that we currently have a BIG problem, and that is the OMAP
> fork being so far out of line with mainline, it isn't funny.

I call it a "branch" myself; "fork" sounds confrontational.

When more of the arch/arm/* core bits merge -- like the clock and
power domain updates ISTR you wanted to hold back -- then the rest
starts to make sense upstream.  Things like board support, and the
drivers needed by those boards.


> There are two approaches to achieve that: take each driver, polish it
> for weeks on end until it's nice and shiney, and then submit it upstream.
> ...
> The other approach is to decide that we have what we have, and that in
> the interests of efficiently reducing divergence, merging the upstream
> changes with the downstream changes and pushing the result upstream ASAP.

Those are two ends of a *spectrum* reflecting how much work to put in,
not two different "approaches".  Between those ends are MANY other
options.  Those two ends are rarely used.

Those options include what I've always thought is the norm:  review
drivers as part of upstream submission, and resolve most issues before
they get merged.  (It's an acknowledged issue that resolving them later
tends to be quite difficult...)


There's also the issue of strategy.  One can randomly take changes
and merge them ... possibly leaving upstream a bit chaotic.  One
could try to merge everything at once ... unrealistic.  One could
merge enough to meet specific functional targets ... which makes
more sense to me.  (Example:  "2.6.28 should boot beagleboard.org
and gumstix Overo from mainline."  That might even be achievable...)


> > If you *really* wanted to avoid wasting time you wouldn't have
> > replied to my previous post, which agreed that one point I raised
> > was a non-issue for this particular patch series.
> 
> Wim said: "Will add patches 1 to 3 when everyone is OK with them. I
> still saw some comments from David."

Yes, there are two unresolved issues in patch #1 which you seem
to have successfully buried with your flamage.  Easy fixes, just
strike a line and truncate a path.  The sort of thing that often
gets queued in the MM tree as a "fixup" and then merged into a
main patch.

- Dave

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[David Brownell]

On Sunday 21 September 2008, Tony Lindgren wrote:
> Hey, you gotta give Dave some credit! Dave's been polishing tons of omap
> code in addition to the USB, at least I2C, gpio, SPI come to mind. Not to
> mention all the blinking leds!

Blinky leds can be the first visible sign of progress!  ;)


> Regarding getting an army of people to fix code, we need to start
> following another standard policy: All drivers must have a MAINTAINER
> who is capable of fixing things, and ideally doing things the right
> way from the start.

Which includes pushing their code into mainline ... and getting
acks (as you noted elsewhere) from the subsystem maintainer to
help avoid going too far astray.

In some cases that implies getting some new frameworks into mainline,
or updating the ones that are there.  The ALSA-SOC stuff has been
a win there, in terms of maintainable code ... the previous solution
involved very little reuse of the funky codec and data stream code,
while now that's far more practical.

Similarly I2C driver model support, GPIO expander infrastructure,
and SPI:  instead of scattering board-specific code in drivers all
over the source tree, it can now be split out fairly cleanly.  That
makes maintainers much happier.

- Dave

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[Russell King - ARM Linux]

On Sun, Sep 21, 2008 at 06:45:42PM -0700, David Brownell wrote:
> Review rarely happens all at once, unless very few people look at
> the code.  Discouraging review is *extremely* strange.

I'm not discouraging review.  I'm saying that making inappropriate
comments isn't helpful.

Yes, your comments are right, but are they appropriate to getting
the OMAP watchdog drivers updated in mainline, or are they more
appropriate in a general sense to all watchdog drivers, and therefore
should be separate from that task?

> > My point is that we currently have a BIG problem, and that is the OMAP
> > fork being so far out of line with mainline, it isn't funny.
> 
> I call it a "branch" myself; "fork" sounds confrontational.

Call it what you want.

> When more of the arch/arm/* core bits merge -- like the clock and
> power domain updates ISTR you wanted to hold back -- then the rest
> starts to make sense upstream.

I never said that - you're twisting my words as normal.

> Yes, there are two unresolved issues in patch #1 which you seem
> to have successfully buried with your flamage.  Easy fixes, just
> strike a line and truncate a path.  The sort of thing that often
> gets queued in the MM tree as a "fixup" and then merged into a
> main patch.

Yet again you use confrontational language, inflaming this discussion.

Okay, I give up.  Folk here can carry on struggling to get their code
into mainline with endless reviews and getting fed up with having to
constantly rework the code over and over again.

Clearly my views aren't welcome.

Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [080922 01:00]:
> On Sun, Sep 21, 2008 at 06:45:42PM -0700, David Brownell wrote:
> > Review rarely happens all at once, unless very few people look at
> > the code.  Discouraging review is *extremely* strange.
> 
> I'm not discouraging review.  I'm saying that making inappropriate
> comments isn't helpful.
> 
> Yes, your comments are right, but are they appropriate to getting
> the OMAP watchdog drivers updated in mainline, or are they more
> appropriate in a general sense to all watchdog drivers, and therefore
> should be separate from that task?
> 
> > > My point is that we currently have a BIG problem, and that is the OMAP
> > > fork being so far out of line with mainline, it isn't funny.
> > 
> > I call it a "branch" myself; "fork" sounds confrontational.
> 
> Call it what you want.
> 
> > When more of the arch/arm/* core bits merge -- like the clock and
> > power domain updates ISTR you wanted to hold back -- then the rest
> > starts to make sense upstream.
> 
> I never said that - you're twisting my words as normal.
> 
> > Yes, there are two unresolved issues in patch #1 which you seem
> > to have successfully buried with your flamage.  Easy fixes, just
> > strike a line and truncate a path.  The sort of thing that often
> > gets queued in the MM tree as a "fixup" and then merged into a
> > main patch.
> 
> Yet again you use confrontational language, inflaming this discussion.
> 
> Okay, I give up.  Folk here can carry on struggling to get their code
> into mainline with endless reviews and getting fed up with having to
> constantly rework the code over and over again.

Hey, please don't give up. You two easily get caught into infinite
mail loops, it's not necessarily omap related ;)

> Clearly my views aren't welcome.

Not true, we _really_ appreciate your comments and help. Same goes for
Dave.

Tony