[PATCH v3 0/11] Xilinx watchdog changes

[PATCH v3 0/11] Xilinx watchdog changes

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

Hi,

This series contains changes for Xilinx watchdog.
The whole code series have been reviewed by Guenter
and device-tree binding was acked by Arnd.

I have also pushed this v3 to git repo
for easier pulling here:
git://git.monstr.eu/linux-2.6-microblaze.git watchdog

Thanks,
Michal

Changes in v3:
- Remove one if checking and use variable directly

Changes in v2:
- Fix enable_once logic
- Change patch subject
- New patch in this series

Michal Simek (11):
  watchdog: xilinx: Convert driver to the watchdog framework
  watchdog: xilinx: Move control_status_reg to functions
  watchdog: xilinx: Simplify probe and remove functions
  watchdog: xilinx: Move no_timeout to probe function
  watchdog: xilinx: Allocate private structure per device
  watchdog: xilinx: Fix all printk messages
  watchdog: xilinx: Use of_property_read_u32
  watchdog: xilinx: Use correct comment indentation
  watchdog: xilinx: Add missing binding
  watchdog: xilinx: Enable this driver for Zynq
  watchdog: xilinx: Remove no_timeout variable

 .../devicetree/bindings/watchdog/of-xilinx-wdt.txt |  23 ++
 drivers/watchdog/Kconfig                           |  21 +-
 drivers/watchdog/of_xilinx_wdt.c                   | 389 ++++++---------------
 3 files changed, 148 insertions(+), 285 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 0/11] Xilinx watchdog changes

[Michal Simek]

Hi,

This series contains changes for Xilinx watchdog.
The whole code series have been reviewed by Guenter
and device-tree binding was acked by Arnd.

I have also pushed this v3 to git repo
for easier pulling here:
git://git.monstr.eu/linux-2.6-microblaze.git watchdog

Thanks,
Michal

Changes in v3:
- Remove one if checking and use variable directly

Changes in v2:
- Fix enable_once logic
- Change patch subject
- New patch in this series

Michal Simek (11):
  watchdog: xilinx: Convert driver to the watchdog framework
  watchdog: xilinx: Move control_status_reg to functions
  watchdog: xilinx: Simplify probe and remove functions
  watchdog: xilinx: Move no_timeout to probe function
  watchdog: xilinx: Allocate private structure per device
  watchdog: xilinx: Fix all printk messages
  watchdog: xilinx: Use of_property_read_u32
  watchdog: xilinx: Use correct comment indentation
  watchdog: xilinx: Add missing binding
  watchdog: xilinx: Enable this driver for Zynq
  watchdog: xilinx: Remove no_timeout variable

 .../devicetree/bindings/watchdog/of-xilinx-wdt.txt |  23 ++
 drivers/watchdog/Kconfig                           |  21 +-
 drivers/watchdog/of_xilinx_wdt.c                   | 389 ++++++---------------
 3 files changed, 148 insertions(+), 285 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt

--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/0c4ba478/attachment.sig>

[PATCH v3 01/11] watchdog: xilinx: Convert driver to the watchdog framework

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 8087 bytes --]

- Remove uneeded headers, fops functions
- Use xilinx_wdt prefix in start/stop/keepalive functions
  and in new structures

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/Kconfig         |   1 +
 drivers/watchdog/of_xilinx_wdt.c | 204 ++++++---------------------------------
 2 files changed, 33 insertions(+), 172 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4c4c566..9db5d3c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1025,6 +1025,7 @@ config M54xx_WATCHDOG
 config XILINX_WATCHDOG
 	tristate "Xilinx Watchdog timer"
 	depends on MICROBLAZE
+	select WATCHDOG_CORE
 	---help---
 	  Watchdog driver for the xps_timebase_wdt ip core.

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index fb57103..8c2814e 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -1,6 +1,7 @@
 /*
  * Watchdog Device Driver for Xilinx axi/xps_timebase_wdt
  *
+ * (C) Copyright 2013 - 2014 Xilinx, Inc.
  * (C) Copyright 2011 (Alejandro Cabrera <aldaya@gmail.com>)
  *
  * This program is free software; you can redistribute it and/or
@@ -14,13 +15,10 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
-#include <linux/fs.h>
-#include <linux/miscdevice.h>
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/watchdog.h>
 #include <linux/io.h>
-#include <linux/uaccess.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_address.h>
@@ -48,22 +46,18 @@
 struct xwdt_device {
 	struct resource  res;
 	void __iomem *base;
-	u32 nowayout;
 	u32 wdt_interval;
-	u32 boot_status;
 };

 static struct xwdt_device xdev;

 static  u32 timeout;
 static  u32 control_status_reg;
-static  u8  expect_close;
 static  u8  no_timeout;
-static unsigned long driver_open;

 static  DEFINE_SPINLOCK(spinlock);

-static void xwdt_start(void)
+static int xilinx_wdt_start(struct watchdog_device *wdd)
 {
 	spin_lock(&spinlock);

@@ -77,9 +71,11 @@ static void xwdt_start(void)
 	iowrite32(XWT_CSRX_EWDT2_MASK, xdev.base + XWT_TWCSR1_OFFSET);

 	spin_unlock(&spinlock);
+
+	return 0;
 }

-static void xwdt_stop(void)
+static int xilinx_wdt_stop(struct watchdog_device *wdd)
 {
 	spin_lock(&spinlock);

@@ -92,9 +88,11 @@ static void xwdt_stop(void)

 	spin_unlock(&spinlock);
 	pr_info("Stopped!\n");
+
+	return 0;
 }

-static void xwdt_keepalive(void)
+static int xilinx_wdt_keepalive(struct watchdog_device *wdd)
 {
 	spin_lock(&spinlock);

@@ -103,23 +101,28 @@ static void xwdt_keepalive(void)
 	iowrite32(control_status_reg, xdev.base + XWT_TWCSR0_OFFSET);

 	spin_unlock(&spinlock);
-}

-static void xwdt_get_status(int *status)
-{
-	int new_status;
+	return 0;
+}

-	spin_lock(&spinlock);
+static const struct watchdog_info xilinx_wdt_ident = {
+	.options =  WDIOF_MAGICCLOSE |
+		    WDIOF_KEEPALIVEPING,
+	.firmware_version =	1,
+	.identity =	WATCHDOG_NAME,
+};

-	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
-	new_status = ((control_status_reg &
-			(XWT_CSR0_WRS_MASK | XWT_CSR0_WDS_MASK)) != 0);
-	spin_unlock(&spinlock);
+static const struct watchdog_ops xilinx_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = xilinx_wdt_start,
+	.stop = xilinx_wdt_stop,
+	.ping = xilinx_wdt_keepalive,
+};

-	*status = 0;
-	if (new_status & 1)
-		*status |= WDIOF_CARDRESET;
-}
+static struct watchdog_device xilinx_wdt_wdd = {
+	.info = &xilinx_wdt_ident,
+	.ops = &xilinx_wdt_ops,
+};

 static u32 xwdt_selftest(void)
 {
@@ -146,139 +149,6 @@ static u32 xwdt_selftest(void)
 		return XWT_TIMER_FAILED;
 }

-static int xwdt_open(struct inode *inode, struct file *file)
-{
-	/* Only one process can handle the wdt at a time */
-	if (test_and_set_bit(0, &driver_open))
-		return -EBUSY;
-
-	/* Make sure that the module are always loaded...*/
-	if (xdev.nowayout)
-		__module_get(THIS_MODULE);
-
-	xwdt_start();
-	pr_info("Started...\n");
-
-	return nonseekable_open(inode, file);
-}
-
-static int xwdt_release(struct inode *inode, struct file *file)
-{
-	if (expect_close == 42) {
-		xwdt_stop();
-	} else {
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-		xwdt_keepalive();
-	}
-
-	clear_bit(0, &driver_open);
-	expect_close = 0;
-	return 0;
-}
-
-/*
- *      xwdt_write:
- *      @file: file handle to the watchdog
- *      @buf: buffer to write (unused as data does not matter here
- *      @count: count of bytes
- *      @ppos: pointer to the position to write. No seeks allowed
- *
- *      A write to a watchdog device is defined as a keepalive signal. Any
- *      write of data will do, as we don't define content meaning.
- */
-static ssize_t xwdt_write(struct file *file, const char __user *buf,
-						size_t len, loff_t *ppos)
-{
-	if (len) {
-		if (!xdev.nowayout) {
-			size_t i;
-
-			/* In case it was set long ago */
-			expect_close = 0;
-
-			for (i = 0; i != len; i++) {
-				char c;
-
-				if (get_user(c, buf + i))
-					return -EFAULT;
-				if (c == 'V')
-					expect_close = 42;
-			}
-		}
-		xwdt_keepalive();
-	}
-	return len;
-}
-
-static const struct watchdog_info ident = {
-	.options =  WDIOF_MAGICCLOSE |
-		    WDIOF_KEEPALIVEPING,
-	.firmware_version =	1,
-	.identity =	WATCHDOG_NAME,
-};
-
-/*
- *      xwdt_ioctl:
- *      @file: file handle to the device
- *      @cmd: watchdog command
- *      @arg: argument pointer
- *
- *      The watchdog API defines a common set of functions for all watchdogs
- *      according to their available features.
- */
-static long xwdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	int status;
-
-	union {
-		struct watchdog_info __user *ident;
-		int __user *i;
-	} uarg;
-
-	uarg.i = (int __user *)arg;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(uarg.ident, &ident,
-					sizeof(ident)) ? -EFAULT : 0;
-
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(xdev.boot_status, uarg.i);
-
-	case WDIOC_GETSTATUS:
-		xwdt_get_status(&status);
-		return put_user(status, uarg.i);
-
-	case WDIOC_KEEPALIVE:
-		xwdt_keepalive();
-		return 0;
-
-	case WDIOC_GETTIMEOUT:
-		if (no_timeout)
-			return -ENOTTY;
-		else
-			return put_user(timeout, uarg.i);
-
-	default:
-		return -ENOTTY;
-	}
-}
-
-static const struct file_operations xwdt_fops = {
-	.owner      = THIS_MODULE,
-	.llseek     = no_llseek,
-	.write      = xwdt_write,
-	.open       = xwdt_open,
-	.release    = xwdt_release,
-	.unlocked_ioctl = xwdt_ioctl,
-};
-
-static struct miscdevice xwdt_miscdev = {
-	.minor      = WATCHDOG_MINOR,
-	.name       = "watchdog",
-	.fops       = &xwdt_fops,
-};
-
 static int xwdt_probe(struct platform_device *pdev)
 {
 	int rc;
@@ -314,7 +184,7 @@ static int xwdt_probe(struct platform_device *pdev)
 					"xlnx,wdt-enable-once", NULL);
 	if (tmptr == NULL) {
 		pr_warn("Parameter \"xlnx,wdt-enable-once\" not found in device tree!\n");
-		xdev.nowayout = WATCHDOG_NOWAYOUT;
+		watchdog_set_nowayout(&xilinx_wdt_wdd, true);
 	}

 /*
@@ -344,24 +214,14 @@ static int xwdt_probe(struct platform_device *pdev)
 		goto unmap_io;
 	}

-	xwdt_get_status(&xdev.boot_status);
-
-	rc = misc_register(&xwdt_miscdev);
+	rc = watchdog_register_device(&xilinx_wdt_wdd);
 	if (rc) {
-		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
-		       xwdt_miscdev.minor, rc);
+		pr_err("cannot register watchdog (err=%d)\n", rc);
 		goto unmap_io;
 	}

-	if (no_timeout)
-		pr_info("driver loaded (timeout=? sec, nowayout=%d)\n",
-			xdev.nowayout);
-	else
-		pr_info("driver loaded (timeout=%d sec, nowayout=%d)\n",
-			timeout, xdev.nowayout);
-
-	expect_close = 0;
-	clear_bit(0, &driver_open);
+	dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds\n",
+		 xdev.base, timeout);

 	return 0;

@@ -375,7 +235,7 @@ err_out:

 static int xwdt_remove(struct platform_device *dev)
 {
-	misc_deregister(&xwdt_miscdev);
+	watchdog_unregister_device(&xilinx_wdt_wdd);
 	iounmap(xdev.base);
 	release_mem_region(xdev.res.start, resource_size(&xdev.res));

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 01/11] watchdog: xilinx: Convert driver to the watchdog framework

[Michal Simek]

- Remove uneeded headers, fops functions
- Use xilinx_wdt prefix in start/stop/keepalive functions
  and in new structures

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/Kconfig         |   1 +
 drivers/watchdog/of_xilinx_wdt.c | 204 ++++++---------------------------------
 2 files changed, 33 insertions(+), 172 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4c4c566..9db5d3c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1025,6 +1025,7 @@ config M54xx_WATCHDOG
 config XILINX_WATCHDOG
 	tristate "Xilinx Watchdog timer"
 	depends on MICROBLAZE
+	select WATCHDOG_CORE
 	---help---
 	  Watchdog driver for the xps_timebase_wdt ip core.

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index fb57103..8c2814e 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -1,6 +1,7 @@
 /*
  * Watchdog Device Driver for Xilinx axi/xps_timebase_wdt
  *
+ * (C) Copyright 2013 - 2014 Xilinx, Inc.
  * (C) Copyright 2011 (Alejandro Cabrera <aldaya@gmail.com>)
  *
  * This program is free software; you can redistribute it and/or
@@ -14,13 +15,10 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
-#include <linux/fs.h>
-#include <linux/miscdevice.h>
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/watchdog.h>
 #include <linux/io.h>
-#include <linux/uaccess.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_address.h>
@@ -48,22 +46,18 @@
 struct xwdt_device {
 	struct resource  res;
 	void __iomem *base;
-	u32 nowayout;
 	u32 wdt_interval;
-	u32 boot_status;
 };

 static struct xwdt_device xdev;

 static  u32 timeout;
 static  u32 control_status_reg;
-static  u8  expect_close;
 static  u8  no_timeout;
-static unsigned long driver_open;

 static  DEFINE_SPINLOCK(spinlock);

-static void xwdt_start(void)
+static int xilinx_wdt_start(struct watchdog_device *wdd)
 {
 	spin_lock(&spinlock);

@@ -77,9 +71,11 @@ static void xwdt_start(void)
 	iowrite32(XWT_CSRX_EWDT2_MASK, xdev.base + XWT_TWCSR1_OFFSET);

 	spin_unlock(&spinlock);
+
+	return 0;
 }

-static void xwdt_stop(void)
+static int xilinx_wdt_stop(struct watchdog_device *wdd)
 {
 	spin_lock(&spinlock);

@@ -92,9 +88,11 @@ static void xwdt_stop(void)

 	spin_unlock(&spinlock);
 	pr_info("Stopped!\n");
+
+	return 0;
 }

-static void xwdt_keepalive(void)
+static int xilinx_wdt_keepalive(struct watchdog_device *wdd)
 {
 	spin_lock(&spinlock);

@@ -103,23 +101,28 @@ static void xwdt_keepalive(void)
 	iowrite32(control_status_reg, xdev.base + XWT_TWCSR0_OFFSET);

 	spin_unlock(&spinlock);
-}

-static void xwdt_get_status(int *status)
-{
-	int new_status;
+	return 0;
+}

-	spin_lock(&spinlock);
+static const struct watchdog_info xilinx_wdt_ident = {
+	.options =  WDIOF_MAGICCLOSE |
+		    WDIOF_KEEPALIVEPING,
+	.firmware_version =	1,
+	.identity =	WATCHDOG_NAME,
+};

-	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
-	new_status = ((control_status_reg &
-			(XWT_CSR0_WRS_MASK | XWT_CSR0_WDS_MASK)) != 0);
-	spin_unlock(&spinlock);
+static const struct watchdog_ops xilinx_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = xilinx_wdt_start,
+	.stop = xilinx_wdt_stop,
+	.ping = xilinx_wdt_keepalive,
+};

-	*status = 0;
-	if (new_status & 1)
-		*status |= WDIOF_CARDRESET;
-}
+static struct watchdog_device xilinx_wdt_wdd = {
+	.info = &xilinx_wdt_ident,
+	.ops = &xilinx_wdt_ops,
+};

 static u32 xwdt_selftest(void)
 {
@@ -146,139 +149,6 @@ static u32 xwdt_selftest(void)
 		return XWT_TIMER_FAILED;
 }

-static int xwdt_open(struct inode *inode, struct file *file)
-{
-	/* Only one process can handle the wdt at a time */
-	if (test_and_set_bit(0, &driver_open))
-		return -EBUSY;
-
-	/* Make sure that the module are always loaded...*/
-	if (xdev.nowayout)
-		__module_get(THIS_MODULE);
-
-	xwdt_start();
-	pr_info("Started...\n");
-
-	return nonseekable_open(inode, file);
-}
-
-static int xwdt_release(struct inode *inode, struct file *file)
-{
-	if (expect_close == 42) {
-		xwdt_stop();
-	} else {
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-		xwdt_keepalive();
-	}
-
-	clear_bit(0, &driver_open);
-	expect_close = 0;
-	return 0;
-}
-
-/*
- *      xwdt_write:
- *      @file: file handle to the watchdog
- *      @buf: buffer to write (unused as data does not matter here
- *      @count: count of bytes
- *      @ppos: pointer to the position to write. No seeks allowed
- *
- *      A write to a watchdog device is defined as a keepalive signal. Any
- *      write of data will do, as we don't define content meaning.
- */
-static ssize_t xwdt_write(struct file *file, const char __user *buf,
-						size_t len, loff_t *ppos)
-{
-	if (len) {
-		if (!xdev.nowayout) {
-			size_t i;
-
-			/* In case it was set long ago */
-			expect_close = 0;
-
-			for (i = 0; i != len; i++) {
-				char c;
-
-				if (get_user(c, buf + i))
-					return -EFAULT;
-				if (c == 'V')
-					expect_close = 42;
-			}
-		}
-		xwdt_keepalive();
-	}
-	return len;
-}
-
-static const struct watchdog_info ident = {
-	.options =  WDIOF_MAGICCLOSE |
-		    WDIOF_KEEPALIVEPING,
-	.firmware_version =	1,
-	.identity =	WATCHDOG_NAME,
-};
-
-/*
- *      xwdt_ioctl:
- *      @file: file handle to the device
- *      @cmd: watchdog command
- *      @arg: argument pointer
- *
- *      The watchdog API defines a common set of functions for all watchdogs
- *      according to their available features.
- */
-static long xwdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	int status;
-
-	union {
-		struct watchdog_info __user *ident;
-		int __user *i;
-	} uarg;
-
-	uarg.i = (int __user *)arg;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(uarg.ident, &ident,
-					sizeof(ident)) ? -EFAULT : 0;
-
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(xdev.boot_status, uarg.i);
-
-	case WDIOC_GETSTATUS:
-		xwdt_get_status(&status);
-		return put_user(status, uarg.i);
-
-	case WDIOC_KEEPALIVE:
-		xwdt_keepalive();
-		return 0;
-
-	case WDIOC_GETTIMEOUT:
-		if (no_timeout)
-			return -ENOTTY;
-		else
-			return put_user(timeout, uarg.i);
-
-	default:
-		return -ENOTTY;
-	}
-}
-
-static const struct file_operations xwdt_fops = {
-	.owner      = THIS_MODULE,
-	.llseek     = no_llseek,
-	.write      = xwdt_write,
-	.open       = xwdt_open,
-	.release    = xwdt_release,
-	.unlocked_ioctl = xwdt_ioctl,
-};
-
-static struct miscdevice xwdt_miscdev = {
-	.minor      = WATCHDOG_MINOR,
-	.name       = "watchdog",
-	.fops       = &xwdt_fops,
-};
-
 static int xwdt_probe(struct platform_device *pdev)
 {
 	int rc;
@@ -314,7 +184,7 @@ static int xwdt_probe(struct platform_device *pdev)
 					"xlnx,wdt-enable-once", NULL);
 	if (tmptr == NULL) {
 		pr_warn("Parameter \"xlnx,wdt-enable-once\" not found in device tree!\n");
-		xdev.nowayout = WATCHDOG_NOWAYOUT;
+		watchdog_set_nowayout(&xilinx_wdt_wdd, true);
 	}

 /*
@@ -344,24 +214,14 @@ static int xwdt_probe(struct platform_device *pdev)
 		goto unmap_io;
 	}

-	xwdt_get_status(&xdev.boot_status);
-
-	rc = misc_register(&xwdt_miscdev);
+	rc = watchdog_register_device(&xilinx_wdt_wdd);
 	if (rc) {
-		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
-		       xwdt_miscdev.minor, rc);
+		pr_err("cannot register watchdog (err=%d)\n", rc);
 		goto unmap_io;
 	}

-	if (no_timeout)
-		pr_info("driver loaded (timeout=? sec, nowayout=%d)\n",
-			xdev.nowayout);
-	else
-		pr_info("driver loaded (timeout=%d sec, nowayout=%d)\n",
-			timeout, xdev.nowayout);
-
-	expect_close = 0;
-	clear_bit(0, &driver_open);
+	dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds\n",
+		 xdev.base, timeout);

 	return 0;

@@ -375,7 +235,7 @@ err_out:

 static int xwdt_remove(struct platform_device *dev)
 {
-	misc_deregister(&xwdt_miscdev);
+	watchdog_unregister_device(&xilinx_wdt_wdd);
 	iounmap(xdev.base);
 	release_mem_region(xdev.res.start, resource_size(&xdev.res));

--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/984695cf/attachment-0001.sig>

[PATCH v3 02/11] watchdog: xilinx: Move control_status_reg to functions

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

control_status_reg is temp variables and should be
used locally by specific function.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 8c2814e..aca9bab 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -52,13 +52,14 @@ struct xwdt_device {
 static struct xwdt_device xdev;

 static  u32 timeout;
-static  u32 control_status_reg;
 static  u8  no_timeout;

 static  DEFINE_SPINLOCK(spinlock);

 static int xilinx_wdt_start(struct watchdog_device *wdd)
 {
+	u32 control_status_reg;
+
 	spin_lock(&spinlock);

 	/* Clean previous status and enable the watchdog timer */
@@ -77,6 +78,8 @@ static int xilinx_wdt_start(struct watchdog_device *wdd)

 static int xilinx_wdt_stop(struct watchdog_device *wdd)
 {
+	u32 control_status_reg;
+
 	spin_lock(&spinlock);

 	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
@@ -94,6 +97,8 @@ static int xilinx_wdt_stop(struct watchdog_device *wdd)

 static int xilinx_wdt_keepalive(struct watchdog_device *wdd)
 {
+	u32 control_status_reg;
+
 	spin_lock(&spinlock);

 	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 02/11] watchdog: xilinx: Move control_status_reg to functions

[Michal Simek]

control_status_reg is temp variables and should be
used locally by specific function.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 8c2814e..aca9bab 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -52,13 +52,14 @@ struct xwdt_device {
 static struct xwdt_device xdev;

 static  u32 timeout;
-static  u32 control_status_reg;
 static  u8  no_timeout;

 static  DEFINE_SPINLOCK(spinlock);

 static int xilinx_wdt_start(struct watchdog_device *wdd)
 {
+	u32 control_status_reg;
+
 	spin_lock(&spinlock);

 	/* Clean previous status and enable the watchdog timer */
@@ -77,6 +78,8 @@ static int xilinx_wdt_start(struct watchdog_device *wdd)

 static int xilinx_wdt_stop(struct watchdog_device *wdd)
 {
+	u32 control_status_reg;
+
 	spin_lock(&spinlock);

 	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
@@ -94,6 +97,8 @@ static int xilinx_wdt_stop(struct watchdog_device *wdd)

 static int xilinx_wdt_keepalive(struct watchdog_device *wdd)
 {
+	u32 control_status_reg;
+
 	spin_lock(&spinlock);

 	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/70480c6a/attachment-0001.sig>

[PATCH v3 03/11] watchdog: xilinx: Simplify probe and remove functions

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]

Use devm_ helper function to simplify probe and error path.
Move ioremap to the beginning of probe function.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 41 +++++++++-------------------------------
 1 file changed, 9 insertions(+), 32 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index aca9bab..7f371ed 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -12,6 +12,7 @@

 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <linux/err.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -44,7 +45,6 @@
 #define PFX WATCHDOG_NAME ": "

 struct xwdt_device {
-	struct resource  res;
 	void __iomem *base;
 	u32 wdt_interval;
 };
@@ -159,9 +159,15 @@ static int xwdt_probe(struct platform_device *pdev)
 	int rc;
 	u32 *tmptr;
 	u32 *pfreq;
+	struct resource *res;

 	no_timeout = 0;

+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	xdev.base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(xdev.base))
+		return PTR_ERR(xdev.base);
+
 	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
 					"clock-frequency", NULL);

@@ -170,12 +176,6 @@ static int xwdt_probe(struct platform_device *pdev)
 		no_timeout = 1;
 	}

-	rc = of_address_to_resource(pdev->dev.of_node, 0, &xdev.res);
-	if (rc) {
-		pr_warn("invalid address!\n");
-		return rc;
-	}
-
 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-interval", NULL);
 	if (tmptr == NULL) {
@@ -199,50 +199,27 @@ static int xwdt_probe(struct platform_device *pdev)
 	if (!no_timeout)
 		timeout = 2 * ((1<<xdev.wdt_interval) / *pfreq);

-	if (!request_mem_region(xdev.res.start,
-			xdev.res.end - xdev.res.start + 1, WATCHDOG_NAME)) {
-		rc = -ENXIO;
-		pr_err("memory request failure!\n");
-		goto err_out;
-	}
-
-	xdev.base = ioremap(xdev.res.start, xdev.res.end - xdev.res.start + 1);
-	if (xdev.base == NULL) {
-		rc = -ENOMEM;
-		pr_err("ioremap failure!\n");
-		goto release_mem;
-	}
-
 	rc = xwdt_selftest();
 	if (rc == XWT_TIMER_FAILED) {
 		pr_err("SelfTest routine error!\n");
-		goto unmap_io;
+		return rc;
 	}

 	rc = watchdog_register_device(&xilinx_wdt_wdd);
 	if (rc) {
 		pr_err("cannot register watchdog (err=%d)\n", rc);
-		goto unmap_io;
+		return rc;
 	}

 	dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds\n",
 		 xdev.base, timeout);

 	return 0;
-
-unmap_io:
-	iounmap(xdev.base);
-release_mem:
-	release_mem_region(xdev.res.start, resource_size(&xdev.res));
-err_out:
-	return rc;
 }

 static int xwdt_remove(struct platform_device *dev)
 {
 	watchdog_unregister_device(&xilinx_wdt_wdd);
-	iounmap(xdev.base);
-	release_mem_region(xdev.res.start, resource_size(&xdev.res));

 	return 0;
 }
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 03/11] watchdog: xilinx: Simplify probe and remove functions

[Michal Simek]

Use devm_ helper function to simplify probe and error path.
Move ioremap to the beginning of probe function.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 41 +++++++++-------------------------------
 1 file changed, 9 insertions(+), 32 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index aca9bab..7f371ed 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -12,6 +12,7 @@

 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <linux/err.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -44,7 +45,6 @@
 #define PFX WATCHDOG_NAME ": "

 struct xwdt_device {
-	struct resource  res;
 	void __iomem *base;
 	u32 wdt_interval;
 };
@@ -159,9 +159,15 @@ static int xwdt_probe(struct platform_device *pdev)
 	int rc;
 	u32 *tmptr;
 	u32 *pfreq;
+	struct resource *res;

 	no_timeout = 0;

+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	xdev.base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(xdev.base))
+		return PTR_ERR(xdev.base);
+
 	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
 					"clock-frequency", NULL);

@@ -170,12 +176,6 @@ static int xwdt_probe(struct platform_device *pdev)
 		no_timeout = 1;
 	}

-	rc = of_address_to_resource(pdev->dev.of_node, 0, &xdev.res);
-	if (rc) {
-		pr_warn("invalid address!\n");
-		return rc;
-	}
-
 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-interval", NULL);
 	if (tmptr == NULL) {
@@ -199,50 +199,27 @@ static int xwdt_probe(struct platform_device *pdev)
 	if (!no_timeout)
 		timeout = 2 * ((1<<xdev.wdt_interval) / *pfreq);

-	if (!request_mem_region(xdev.res.start,
-			xdev.res.end - xdev.res.start + 1, WATCHDOG_NAME)) {
-		rc = -ENXIO;
-		pr_err("memory request failure!\n");
-		goto err_out;
-	}
-
-	xdev.base = ioremap(xdev.res.start, xdev.res.end - xdev.res.start + 1);
-	if (xdev.base == NULL) {
-		rc = -ENOMEM;
-		pr_err("ioremap failure!\n");
-		goto release_mem;
-	}
-
 	rc = xwdt_selftest();
 	if (rc == XWT_TIMER_FAILED) {
 		pr_err("SelfTest routine error!\n");
-		goto unmap_io;
+		return rc;
 	}

 	rc = watchdog_register_device(&xilinx_wdt_wdd);
 	if (rc) {
 		pr_err("cannot register watchdog (err=%d)\n", rc);
-		goto unmap_io;
+		return rc;
 	}

 	dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds\n",
 		 xdev.base, timeout);

 	return 0;
-
-unmap_io:
-	iounmap(xdev.base);
-release_mem:
-	release_mem_region(xdev.res.start, resource_size(&xdev.res));
-err_out:
-	return rc;
 }

 static int xwdt_remove(struct platform_device *dev)
 {
 	watchdog_unregister_device(&xilinx_wdt_wdd);
-	iounmap(xdev.base);
-	release_mem_region(xdev.res.start, resource_size(&xdev.res));

 	return 0;
 }
--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/f8563f2e/attachment.sig>

[PATCH v3 04/11] watchdog: xilinx: Move no_timeout to probe function

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

no_timeout should be local variable because it is used
only in probe function.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 7f371ed..1f7ad91 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -52,7 +52,6 @@ struct xwdt_device {
 static struct xwdt_device xdev;

 static  u32 timeout;
-static  u8  no_timeout;

 static  DEFINE_SPINLOCK(spinlock);

@@ -160,8 +159,7 @@ static int xwdt_probe(struct platform_device *pdev)
 	u32 *tmptr;
 	u32 *pfreq;
 	struct resource *res;
-
-	no_timeout = 0;
+	bool no_timeout = false;

 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	xdev.base = devm_ioremap_resource(&pdev->dev, res);
@@ -173,14 +171,14 @@ static int xwdt_probe(struct platform_device *pdev)

 	if (pfreq == NULL) {
 		pr_warn("The watchdog clock frequency cannot be obtained!\n");
-		no_timeout = 1;
+		no_timeout = true;
 	}

 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-interval", NULL);
 	if (tmptr == NULL) {
 		pr_warn("Parameter \"xlnx,wdt-interval\" not found in device tree!\n");
-		no_timeout = 1;
+		no_timeout = true;
 	} else {
 		xdev.wdt_interval = *tmptr;
 	}
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 04/11] watchdog: xilinx: Move no_timeout to probe function

[Michal Simek]

no_timeout should be local variable because it is used
only in probe function.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 7f371ed..1f7ad91 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -52,7 +52,6 @@ struct xwdt_device {
 static struct xwdt_device xdev;

 static  u32 timeout;
-static  u8  no_timeout;

 static  DEFINE_SPINLOCK(spinlock);

@@ -160,8 +159,7 @@ static int xwdt_probe(struct platform_device *pdev)
 	u32 *tmptr;
 	u32 *pfreq;
 	struct resource *res;
-
-	no_timeout = 0;
+	bool no_timeout = false;

 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	xdev.base = devm_ioremap_resource(&pdev->dev, res);
@@ -173,14 +171,14 @@ static int xwdt_probe(struct platform_device *pdev)

 	if (pfreq == NULL) {
 		pr_warn("The watchdog clock frequency cannot be obtained!\n");
-		no_timeout = 1;
+		no_timeout = true;
 	}

 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-interval", NULL);
 	if (tmptr == NULL) {
 		pr_warn("Parameter \"xlnx,wdt-interval\" not found in device tree!\n");
-		no_timeout = 1;
+		no_timeout = true;
 	} else {
 		xdev.wdt_interval = *tmptr;
 	}
--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/334443b8/attachment.sig>

[PATCH v3 05/11] watchdog: xilinx: Allocate private structure per device

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 6631 bytes --]

Only one watchdog could be used by this driver.
Create driver private data structure and move there
all variables for one instance.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 97 +++++++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 42 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 1f7ad91..d28bd3f 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -47,30 +47,27 @@
 struct xwdt_device {
 	void __iomem *base;
 	u32 wdt_interval;
+	spinlock_t spinlock;
+	struct watchdog_device xilinx_wdt_wdd;
 };

-static struct xwdt_device xdev;
-
-static  u32 timeout;
-
-static  DEFINE_SPINLOCK(spinlock);
-
 static int xilinx_wdt_start(struct watchdog_device *wdd)
 {
 	u32 control_status_reg;
+	struct xwdt_device *xdev = watchdog_get_drvdata(wdd);

-	spin_lock(&spinlock);
+	spin_lock(&xdev->spinlock);

 	/* Clean previous status and enable the watchdog timer */
-	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
+	control_status_reg = ioread32(xdev->base + XWT_TWCSR0_OFFSET);
 	control_status_reg |= (XWT_CSR0_WRS_MASK | XWT_CSR0_WDS_MASK);

 	iowrite32((control_status_reg | XWT_CSR0_EWDT1_MASK),
-				xdev.base + XWT_TWCSR0_OFFSET);
+		  xdev->base + XWT_TWCSR0_OFFSET);

-	iowrite32(XWT_CSRX_EWDT2_MASK, xdev.base + XWT_TWCSR1_OFFSET);
+	iowrite32(XWT_CSRX_EWDT2_MASK, xdev->base + XWT_TWCSR1_OFFSET);

-	spin_unlock(&spinlock);
+	spin_unlock(&xdev->spinlock);

 	return 0;
 }
@@ -78,17 +75,18 @@ static int xilinx_wdt_start(struct watchdog_device *wdd)
 static int xilinx_wdt_stop(struct watchdog_device *wdd)
 {
 	u32 control_status_reg;
+	struct xwdt_device *xdev = watchdog_get_drvdata(wdd);

-	spin_lock(&spinlock);
+	spin_lock(&xdev->spinlock);

-	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
+	control_status_reg = ioread32(xdev->base + XWT_TWCSR0_OFFSET);

 	iowrite32((control_status_reg & ~XWT_CSR0_EWDT1_MASK),
-				xdev.base + XWT_TWCSR0_OFFSET);
+		  xdev->base + XWT_TWCSR0_OFFSET);

-	iowrite32(0, xdev.base + XWT_TWCSR1_OFFSET);
+	iowrite32(0, xdev->base + XWT_TWCSR1_OFFSET);

-	spin_unlock(&spinlock);
+	spin_unlock(&xdev->spinlock);
 	pr_info("Stopped!\n");

 	return 0;
@@ -97,14 +95,15 @@ static int xilinx_wdt_stop(struct watchdog_device *wdd)
 static int xilinx_wdt_keepalive(struct watchdog_device *wdd)
 {
 	u32 control_status_reg;
+	struct xwdt_device *xdev = watchdog_get_drvdata(wdd);

-	spin_lock(&spinlock);
+	spin_lock(&xdev->spinlock);

-	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
+	control_status_reg = ioread32(xdev->base + XWT_TWCSR0_OFFSET);
 	control_status_reg |= (XWT_CSR0_WRS_MASK | XWT_CSR0_WDS_MASK);
-	iowrite32(control_status_reg, xdev.base + XWT_TWCSR0_OFFSET);
+	iowrite32(control_status_reg, xdev->base + XWT_TWCSR0_OFFSET);

-	spin_unlock(&spinlock);
+	spin_unlock(&xdev->spinlock);

 	return 0;
 }
@@ -123,29 +122,24 @@ static const struct watchdog_ops xilinx_wdt_ops = {
 	.ping = xilinx_wdt_keepalive,
 };

-static struct watchdog_device xilinx_wdt_wdd = {
-	.info = &xilinx_wdt_ident,
-	.ops = &xilinx_wdt_ops,
-};
-
-static u32 xwdt_selftest(void)
+static u32 xwdt_selftest(struct xwdt_device *xdev)
 {
 	int i;
 	u32 timer_value1;
 	u32 timer_value2;

-	spin_lock(&spinlock);
+	spin_lock(&xdev->spinlock);

-	timer_value1 = ioread32(xdev.base + XWT_TBR_OFFSET);
-	timer_value2 = ioread32(xdev.base + XWT_TBR_OFFSET);
+	timer_value1 = ioread32(xdev->base + XWT_TBR_OFFSET);
+	timer_value2 = ioread32(xdev->base + XWT_TBR_OFFSET);

 	for (i = 0;
 		((i <= XWT_MAX_SELFTEST_LOOP_COUNT) &&
 			(timer_value2 == timer_value1)); i++) {
-		timer_value2 = ioread32(xdev.base + XWT_TBR_OFFSET);
+		timer_value2 = ioread32(xdev->base + XWT_TBR_OFFSET);
 	}

-	spin_unlock(&spinlock);
+	spin_unlock(&xdev->spinlock);

 	if (timer_value2 != timer_value1)
 		return ~XWT_TIMER_FAILED;
@@ -159,12 +153,23 @@ static int xwdt_probe(struct platform_device *pdev)
 	u32 *tmptr;
 	u32 *pfreq;
 	struct resource *res;
+	struct xwdt_device *xdev;
 	bool no_timeout = false;
+	struct watchdog_device *xilinx_wdt_wdd;
+
+	xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL);
+	if (!xdev)
+		return -ENOMEM;
+
+	xilinx_wdt_wdd = &xdev->xilinx_wdt_wdd;
+	xilinx_wdt_wdd->info = &xilinx_wdt_ident;
+	xilinx_wdt_wdd->ops = &xilinx_wdt_ops;
+	xilinx_wdt_wdd->parent = &pdev->dev;

 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	xdev.base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(xdev.base))
-		return PTR_ERR(xdev.base);
+	xdev->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(xdev->base))
+		return PTR_ERR(xdev->base);

 	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
 					"clock-frequency", NULL);
@@ -180,14 +185,14 @@ static int xwdt_probe(struct platform_device *pdev)
 		pr_warn("Parameter \"xlnx,wdt-interval\" not found in device tree!\n");
 		no_timeout = true;
 	} else {
-		xdev.wdt_interval = *tmptr;
+		xdev->wdt_interval = *tmptr;
 	}

 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-enable-once", NULL);
 	if (tmptr == NULL) {
 		pr_warn("Parameter \"xlnx,wdt-enable-once\" not found in device tree!\n");
-		watchdog_set_nowayout(&xilinx_wdt_wdd, true);
+		watchdog_set_nowayout(xilinx_wdt_wdd, true);
 	}

 /*
@@ -195,29 +200,37 @@ static int xwdt_probe(struct platform_device *pdev)
  *  ignored (interrupt), reset is only generated at second wdt overflow
  */
 	if (!no_timeout)
-		timeout = 2 * ((1<<xdev.wdt_interval) / *pfreq);
+		xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
+					  *pfreq);
+
+	spin_lock_init(&xdev->spinlock);
+	watchdog_set_drvdata(xilinx_wdt_wdd, xdev);

-	rc = xwdt_selftest();
+	rc = xwdt_selftest(xdev);
 	if (rc == XWT_TIMER_FAILED) {
 		pr_err("SelfTest routine error!\n");
 		return rc;
 	}

-	rc = watchdog_register_device(&xilinx_wdt_wdd);
+	rc = watchdog_register_device(xilinx_wdt_wdd);
 	if (rc) {
 		pr_err("cannot register watchdog (err=%d)\n", rc);
 		return rc;
 	}

 	dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds\n",
-		 xdev.base, timeout);
+		 xdev->base, xilinx_wdt_wdd->timeout);
+
+	platform_set_drvdata(pdev, xdev);

 	return 0;
 }

-static int xwdt_remove(struct platform_device *dev)
+static int xwdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&xilinx_wdt_wdd);
+	struct xwdt_device *xdev = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&xdev->xilinx_wdt_wdd);

 	return 0;
 }
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 05/11] watchdog: xilinx: Allocate private structure per device

[Michal Simek]

Only one watchdog could be used by this driver.
Create driver private data structure and move there
all variables for one instance.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 97 +++++++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 42 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 1f7ad91..d28bd3f 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -47,30 +47,27 @@
 struct xwdt_device {
 	void __iomem *base;
 	u32 wdt_interval;
+	spinlock_t spinlock;
+	struct watchdog_device xilinx_wdt_wdd;
 };

-static struct xwdt_device xdev;
-
-static  u32 timeout;
-
-static  DEFINE_SPINLOCK(spinlock);
-
 static int xilinx_wdt_start(struct watchdog_device *wdd)
 {
 	u32 control_status_reg;
+	struct xwdt_device *xdev = watchdog_get_drvdata(wdd);

-	spin_lock(&spinlock);
+	spin_lock(&xdev->spinlock);

 	/* Clean previous status and enable the watchdog timer */
-	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
+	control_status_reg = ioread32(xdev->base + XWT_TWCSR0_OFFSET);
 	control_status_reg |= (XWT_CSR0_WRS_MASK | XWT_CSR0_WDS_MASK);

 	iowrite32((control_status_reg | XWT_CSR0_EWDT1_MASK),
-				xdev.base + XWT_TWCSR0_OFFSET);
+		  xdev->base + XWT_TWCSR0_OFFSET);

-	iowrite32(XWT_CSRX_EWDT2_MASK, xdev.base + XWT_TWCSR1_OFFSET);
+	iowrite32(XWT_CSRX_EWDT2_MASK, xdev->base + XWT_TWCSR1_OFFSET);

-	spin_unlock(&spinlock);
+	spin_unlock(&xdev->spinlock);

 	return 0;
 }
@@ -78,17 +75,18 @@ static int xilinx_wdt_start(struct watchdog_device *wdd)
 static int xilinx_wdt_stop(struct watchdog_device *wdd)
 {
 	u32 control_status_reg;
+	struct xwdt_device *xdev = watchdog_get_drvdata(wdd);

-	spin_lock(&spinlock);
+	spin_lock(&xdev->spinlock);

-	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
+	control_status_reg = ioread32(xdev->base + XWT_TWCSR0_OFFSET);

 	iowrite32((control_status_reg & ~XWT_CSR0_EWDT1_MASK),
-				xdev.base + XWT_TWCSR0_OFFSET);
+		  xdev->base + XWT_TWCSR0_OFFSET);

-	iowrite32(0, xdev.base + XWT_TWCSR1_OFFSET);
+	iowrite32(0, xdev->base + XWT_TWCSR1_OFFSET);

-	spin_unlock(&spinlock);
+	spin_unlock(&xdev->spinlock);
 	pr_info("Stopped!\n");

 	return 0;
@@ -97,14 +95,15 @@ static int xilinx_wdt_stop(struct watchdog_device *wdd)
 static int xilinx_wdt_keepalive(struct watchdog_device *wdd)
 {
 	u32 control_status_reg;
+	struct xwdt_device *xdev = watchdog_get_drvdata(wdd);

-	spin_lock(&spinlock);
+	spin_lock(&xdev->spinlock);

-	control_status_reg = ioread32(xdev.base + XWT_TWCSR0_OFFSET);
+	control_status_reg = ioread32(xdev->base + XWT_TWCSR0_OFFSET);
 	control_status_reg |= (XWT_CSR0_WRS_MASK | XWT_CSR0_WDS_MASK);
-	iowrite32(control_status_reg, xdev.base + XWT_TWCSR0_OFFSET);
+	iowrite32(control_status_reg, xdev->base + XWT_TWCSR0_OFFSET);

-	spin_unlock(&spinlock);
+	spin_unlock(&xdev->spinlock);

 	return 0;
 }
@@ -123,29 +122,24 @@ static const struct watchdog_ops xilinx_wdt_ops = {
 	.ping = xilinx_wdt_keepalive,
 };

-static struct watchdog_device xilinx_wdt_wdd = {
-	.info = &xilinx_wdt_ident,
-	.ops = &xilinx_wdt_ops,
-};
-
-static u32 xwdt_selftest(void)
+static u32 xwdt_selftest(struct xwdt_device *xdev)
 {
 	int i;
 	u32 timer_value1;
 	u32 timer_value2;

-	spin_lock(&spinlock);
+	spin_lock(&xdev->spinlock);

-	timer_value1 = ioread32(xdev.base + XWT_TBR_OFFSET);
-	timer_value2 = ioread32(xdev.base + XWT_TBR_OFFSET);
+	timer_value1 = ioread32(xdev->base + XWT_TBR_OFFSET);
+	timer_value2 = ioread32(xdev->base + XWT_TBR_OFFSET);

 	for (i = 0;
 		((i <= XWT_MAX_SELFTEST_LOOP_COUNT) &&
 			(timer_value2 == timer_value1)); i++) {
-		timer_value2 = ioread32(xdev.base + XWT_TBR_OFFSET);
+		timer_value2 = ioread32(xdev->base + XWT_TBR_OFFSET);
 	}

-	spin_unlock(&spinlock);
+	spin_unlock(&xdev->spinlock);

 	if (timer_value2 != timer_value1)
 		return ~XWT_TIMER_FAILED;
@@ -159,12 +153,23 @@ static int xwdt_probe(struct platform_device *pdev)
 	u32 *tmptr;
 	u32 *pfreq;
 	struct resource *res;
+	struct xwdt_device *xdev;
 	bool no_timeout = false;
+	struct watchdog_device *xilinx_wdt_wdd;
+
+	xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL);
+	if (!xdev)
+		return -ENOMEM;
+
+	xilinx_wdt_wdd = &xdev->xilinx_wdt_wdd;
+	xilinx_wdt_wdd->info = &xilinx_wdt_ident;
+	xilinx_wdt_wdd->ops = &xilinx_wdt_ops;
+	xilinx_wdt_wdd->parent = &pdev->dev;

 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	xdev.base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(xdev.base))
-		return PTR_ERR(xdev.base);
+	xdev->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(xdev->base))
+		return PTR_ERR(xdev->base);

 	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
 					"clock-frequency", NULL);
@@ -180,14 +185,14 @@ static int xwdt_probe(struct platform_device *pdev)
 		pr_warn("Parameter \"xlnx,wdt-interval\" not found in device tree!\n");
 		no_timeout = true;
 	} else {
-		xdev.wdt_interval = *tmptr;
+		xdev->wdt_interval = *tmptr;
 	}

 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-enable-once", NULL);
 	if (tmptr == NULL) {
 		pr_warn("Parameter \"xlnx,wdt-enable-once\" not found in device tree!\n");
-		watchdog_set_nowayout(&xilinx_wdt_wdd, true);
+		watchdog_set_nowayout(xilinx_wdt_wdd, true);
 	}

 /*
@@ -195,29 +200,37 @@ static int xwdt_probe(struct platform_device *pdev)
  *  ignored (interrupt), reset is only generated at second wdt overflow
  */
 	if (!no_timeout)
-		timeout = 2 * ((1<<xdev.wdt_interval) / *pfreq);
+		xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
+					  *pfreq);
+
+	spin_lock_init(&xdev->spinlock);
+	watchdog_set_drvdata(xilinx_wdt_wdd, xdev);

-	rc = xwdt_selftest();
+	rc = xwdt_selftest(xdev);
 	if (rc == XWT_TIMER_FAILED) {
 		pr_err("SelfTest routine error!\n");
 		return rc;
 	}

-	rc = watchdog_register_device(&xilinx_wdt_wdd);
+	rc = watchdog_register_device(xilinx_wdt_wdd);
 	if (rc) {
 		pr_err("cannot register watchdog (err=%d)\n", rc);
 		return rc;
 	}

 	dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds\n",
-		 xdev.base, timeout);
+		 xdev->base, xilinx_wdt_wdd->timeout);
+
+	platform_set_drvdata(pdev, xdev);

 	return 0;
 }

-static int xwdt_remove(struct platform_device *dev)
+static int xwdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&xilinx_wdt_wdd);
+	struct xwdt_device *xdev = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&xdev->xilinx_wdt_wdd);

 	return 0;
 }
--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/8a578026/attachment.sig>

[PATCH v3 06/11] watchdog: xilinx: Fix all printk messages

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

Use dev_ functions for printk messages.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index d28bd3f..c229cc4 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -10,8 +10,6 @@
  * 2 of the License, or (at your option) any later version.
  */

-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -42,7 +40,6 @@
 #define XWT_TIMER_FAILED            0xFFFFFFFF

 #define WATCHDOG_NAME     "Xilinx Watchdog"
-#define PFX WATCHDOG_NAME ": "

 struct xwdt_device {
 	void __iomem *base;
@@ -175,14 +172,16 @@ static int xwdt_probe(struct platform_device *pdev)
 					"clock-frequency", NULL);

 	if (pfreq == NULL) {
-		pr_warn("The watchdog clock frequency cannot be obtained!\n");
+		dev_warn(&pdev->dev,
+			 "The watchdog clock frequency cannot be obtained\n");
 		no_timeout = true;
 	}

 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-interval", NULL);
 	if (tmptr == NULL) {
-		pr_warn("Parameter \"xlnx,wdt-interval\" not found in device tree!\n");
+		dev_warn(&pdev->dev,
+			 "Parameter \"xlnx,wdt-interval\" not found\n");
 		no_timeout = true;
 	} else {
 		xdev->wdt_interval = *tmptr;
@@ -191,7 +190,8 @@ static int xwdt_probe(struct platform_device *pdev)
 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-enable-once", NULL);
 	if (tmptr == NULL) {
-		pr_warn("Parameter \"xlnx,wdt-enable-once\" not found in device tree!\n");
+		dev_warn(&pdev->dev,
+			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
 		watchdog_set_nowayout(xilinx_wdt_wdd, true);
 	}

@@ -208,13 +208,13 @@ static int xwdt_probe(struct platform_device *pdev)

 	rc = xwdt_selftest(xdev);
 	if (rc == XWT_TIMER_FAILED) {
-		pr_err("SelfTest routine error!\n");
+		dev_err(&pdev->dev, "SelfTest routine error\n");
 		return rc;
 	}

 	rc = watchdog_register_device(xilinx_wdt_wdd);
 	if (rc) {
-		pr_err("cannot register watchdog (err=%d)\n", rc);
+		dev_err(&pdev->dev, "Cannot register watchdog (err=%d)\n", rc);
 		return rc;
 	}

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 06/11] watchdog: xilinx: Fix all printk messages

[Michal Simek]

Use dev_ functions for printk messages.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index d28bd3f..c229cc4 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -10,8 +10,6 @@
  * 2 of the License, or (at your option) any later version.
  */

-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -42,7 +40,6 @@
 #define XWT_TIMER_FAILED            0xFFFFFFFF

 #define WATCHDOG_NAME     "Xilinx Watchdog"
-#define PFX WATCHDOG_NAME ": "

 struct xwdt_device {
 	void __iomem *base;
@@ -175,14 +172,16 @@ static int xwdt_probe(struct platform_device *pdev)
 					"clock-frequency", NULL);

 	if (pfreq == NULL) {
-		pr_warn("The watchdog clock frequency cannot be obtained!\n");
+		dev_warn(&pdev->dev,
+			 "The watchdog clock frequency cannot be obtained\n");
 		no_timeout = true;
 	}

 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-interval", NULL);
 	if (tmptr == NULL) {
-		pr_warn("Parameter \"xlnx,wdt-interval\" not found in device tree!\n");
+		dev_warn(&pdev->dev,
+			 "Parameter \"xlnx,wdt-interval\" not found\n");
 		no_timeout = true;
 	} else {
 		xdev->wdt_interval = *tmptr;
@@ -191,7 +190,8 @@ static int xwdt_probe(struct platform_device *pdev)
 	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
 					"xlnx,wdt-enable-once", NULL);
 	if (tmptr == NULL) {
-		pr_warn("Parameter \"xlnx,wdt-enable-once\" not found in device tree!\n");
+		dev_warn(&pdev->dev,
+			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
 		watchdog_set_nowayout(xilinx_wdt_wdd, true);
 	}

@@ -208,13 +208,13 @@ static int xwdt_probe(struct platform_device *pdev)

 	rc = xwdt_selftest(xdev);
 	if (rc == XWT_TIMER_FAILED) {
-		pr_err("SelfTest routine error!\n");
+		dev_err(&pdev->dev, "SelfTest routine error\n");
 		return rc;
 	}

 	rc = watchdog_register_device(xilinx_wdt_wdd);
 	if (rc) {
-		pr_err("cannot register watchdog (err=%d)\n", rc);
+		dev_err(&pdev->dev, "Cannot register watchdog (err=%d)\n", rc);
 		return rc;
 	}

--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/7a46ccec/attachment-0001.sig>

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

Use of_property_read_u32 functions to clean probe function.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3:
- Remove one if checking and use variable directly

Changes in v2:
- Fix enable_once logic
- Change patch subject

 drivers/watchdog/of_xilinx_wdt.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index c229cc4..7deea21 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev)
 static int xwdt_probe(struct platform_device *pdev)
 {
 	int rc;
-	u32 *tmptr;
-	u32 *pfreq;
+	u32 pfreq, enable_once = 0;
 	struct resource *res;
 	struct xwdt_device *xdev;
 	bool no_timeout = false;
@@ -168,32 +167,28 @@ static int xwdt_probe(struct platform_device *pdev)
 	if (IS_ERR(xdev->base))
 		return PTR_ERR(xdev->base);

-	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
-					"clock-frequency", NULL);
-
-	if (pfreq == NULL) {
+	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
+	if (rc) {
 		dev_warn(&pdev->dev,
 			 "The watchdog clock frequency cannot be obtained\n");
 		no_timeout = true;
 	}

-	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
-					"xlnx,wdt-interval", NULL);
-	if (tmptr == NULL) {
+	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
+				  &xdev->wdt_interval);
+	if (rc) {
 		dev_warn(&pdev->dev,
 			 "Parameter \"xlnx,wdt-interval\" not found\n");
 		no_timeout = true;
-	} else {
-		xdev->wdt_interval = *tmptr;
 	}

-	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
-					"xlnx,wdt-enable-once", NULL);
-	if (tmptr == NULL) {
+	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
+				  &enable_once);
+	if (rc)
 		dev_warn(&pdev->dev,
 			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
-		watchdog_set_nowayout(xilinx_wdt_wdd, true);
-	}
+
+	watchdog_set_nowayout(xilinx_wdt_wdd, enable_once);

 /*
  *  Twice of the 2^wdt_interval / freq  because the first wdt overflow is
@@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev)
  */
 	if (!no_timeout)
 		xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
-					  *pfreq);
+					  pfreq);

 	spin_lock_init(&xdev->spinlock);
 	watchdog_set_drvdata(xilinx_wdt_wdd, xdev);
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Michal Simek]

Use of_property_read_u32 functions to clean probe function.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3:
- Remove one if checking and use variable directly

Changes in v2:
- Fix enable_once logic
- Change patch subject

 drivers/watchdog/of_xilinx_wdt.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index c229cc4..7deea21 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev)
 static int xwdt_probe(struct platform_device *pdev)
 {
 	int rc;
-	u32 *tmptr;
-	u32 *pfreq;
+	u32 pfreq, enable_once = 0;
 	struct resource *res;
 	struct xwdt_device *xdev;
 	bool no_timeout = false;
@@ -168,32 +167,28 @@ static int xwdt_probe(struct platform_device *pdev)
 	if (IS_ERR(xdev->base))
 		return PTR_ERR(xdev->base);

-	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
-					"clock-frequency", NULL);
-
-	if (pfreq == NULL) {
+	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
+	if (rc) {
 		dev_warn(&pdev->dev,
 			 "The watchdog clock frequency cannot be obtained\n");
 		no_timeout = true;
 	}

-	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
-					"xlnx,wdt-interval", NULL);
-	if (tmptr == NULL) {
+	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
+				  &xdev->wdt_interval);
+	if (rc) {
 		dev_warn(&pdev->dev,
 			 "Parameter \"xlnx,wdt-interval\" not found\n");
 		no_timeout = true;
-	} else {
-		xdev->wdt_interval = *tmptr;
 	}

-	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
-					"xlnx,wdt-enable-once", NULL);
-	if (tmptr == NULL) {
+	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
+				  &enable_once);
+	if (rc)
 		dev_warn(&pdev->dev,
 			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
-		watchdog_set_nowayout(xilinx_wdt_wdd, true);
-	}
+
+	watchdog_set_nowayout(xilinx_wdt_wdd, enable_once);

 /*
  *  Twice of the 2^wdt_interval / freq  because the first wdt overflow is
@@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev)
  */
 	if (!no_timeout)
 		xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
-					  *pfreq);
+					  pfreq);

 	spin_lock_init(&xdev->spinlock);
 	watchdog_set_drvdata(xilinx_wdt_wdd, xdev);
--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/7438f5b3/attachment.sig>

[PATCH v3 08/11] watchdog: xilinx: Use correct comment indentation

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

No functional changes.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 7deea21..9dd16cf 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -190,10 +190,10 @@ static int xwdt_probe(struct platform_device *pdev)

 	watchdog_set_nowayout(xilinx_wdt_wdd, enable_once);

-/*
- *  Twice of the 2^wdt_interval / freq  because the first wdt overflow is
- *  ignored (interrupt), reset is only generated at second wdt overflow
- */
+	/*
+	 * Twice of the 2^wdt_interval / freq  because the first wdt overflow is
+	 * ignored (interrupt), reset is only generated at second wdt overflow
+	 */
 	if (!no_timeout)
 		xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
 					  pfreq);
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 08/11] watchdog: xilinx: Use correct comment indentation

[Michal Simek]

No functional changes.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

 drivers/watchdog/of_xilinx_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 7deea21..9dd16cf 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -190,10 +190,10 @@ static int xwdt_probe(struct platform_device *pdev)

 	watchdog_set_nowayout(xilinx_wdt_wdd, enable_once);

-/*
- *  Twice of the 2^wdt_interval / freq  because the first wdt overflow is
- *  ignored (interrupt), reset is only generated at second wdt overflow
- */
+	/*
+	 * Twice of the 2^wdt_interval / freq  because the first wdt overflow is
+	 * ignored (interrupt), reset is only generated at second wdt overflow
+	 */
 	if (!no_timeout)
 		xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
 					  pfreq);
--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/35b68b9c/attachment.sig>

[PATCH v3 09/11] watchdog: xilinx: Add missing binding

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

Document current driver binding.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---

Changes in v3: None
Changes in v2: None

 .../devicetree/bindings/watchdog/of-xilinx-wdt.txt | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
new file mode 100644
index 0000000..6d63782
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
@@ -0,0 +1,23 @@
+Xilinx AXI/PLB soft-core watchdog Device Tree Bindings
+---------------------------------------------------------
+
+Required properties:
+- compatible		: Should be "xlnx,xps-timebase-wdt-1.00.a" or
+			  "xlnx,xps-timebase-wdt-1.01.a".
+- reg			: Physical base address and size
+
+Optional properties:
+- clock-frequency	: Frequency of clock in Hz
+- xlnx,wdt-enable-once	: 0 - Watchdog can be restarted
+			  1 - Watchdog can be enabled just once
+- xlnx,wdt-interval	: Watchdog timeout interval in 2^<val> clock cycles,
+			  <val> is integer from 8 to 31.
+
+Example:
+axi-timebase-wdt@40100000 {
+	clock-frequency = <50000000>;
+	compatible = "xlnx,xps-timebase-wdt-1.00.a";
+	reg = <0x40100000 0x10000>;
+	xlnx,wdt-enable-once = <0x0>;
+	xlnx,wdt-interval = <0x1b>;
+} ;
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 09/11] watchdog: xilinx: Add missing binding

[Michal Simek]

Document current driver binding.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---

Changes in v3: None
Changes in v2: None

 .../devicetree/bindings/watchdog/of-xilinx-wdt.txt | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
new file mode 100644
index 0000000..6d63782
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
@@ -0,0 +1,23 @@
+Xilinx AXI/PLB soft-core watchdog Device Tree Bindings
+---------------------------------------------------------
+
+Required properties:
+- compatible		: Should be "xlnx,xps-timebase-wdt-1.00.a" or
+			  "xlnx,xps-timebase-wdt-1.01.a".
+- reg			: Physical base address and size
+
+Optional properties:
+- clock-frequency	: Frequency of clock in Hz
+- xlnx,wdt-enable-once	: 0 - Watchdog can be restarted
+			  1 - Watchdog can be enabled just once
+- xlnx,wdt-interval	: Watchdog timeout interval in 2^<val> clock cycles,
+			  <val> is integer from 8 to 31.
+
+Example:
+axi-timebase-wdt at 40100000 {
+	clock-frequency = <50000000>;
+	compatible = "xlnx,xps-timebase-wdt-1.00.a";
+	reg = <0x40100000 0x10000>;
+	xlnx,wdt-enable-once = <0x0>;
+	xlnx,wdt-interval = <0x1b>;
+} ;
--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/ee768c84/attachment.sig>

[PATCH v3 10/11] watchdog: xilinx: Enable this driver for Zynq

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

Enable this driver for Zynq.
Move it to architecture independent Kconfig part.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

Build tested by zero day testing system.
---
 drivers/watchdog/Kconfig | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9db5d3c..6120403 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -111,6 +111,15 @@ config WM8350_WATCHDOG
 	  Support for the watchdog in the WM8350 AudioPlus PMIC.  When
 	  the watchdog triggers the system will be reset.

+config XILINX_WATCHDOG
+	tristate "Xilinx Watchdog timer"
+	select WATCHDOG_CORE
+	help
+	  Watchdog driver for the xps_timebase_wdt ip core.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called of_xilinx_wdt.
+
 # ALPHA Architecture

 # ARM Architecture
@@ -1022,19 +1031,6 @@ config M54xx_WATCHDOG

 # MicroBlaze Architecture

-config XILINX_WATCHDOG
-	tristate "Xilinx Watchdog timer"
-	depends on MICROBLAZE
-	select WATCHDOG_CORE
-	---help---
-	  Watchdog driver for the xps_timebase_wdt ip core.
-
-	  IMPORTANT: The xps_timebase_wdt parent must have the property
-	  "clock-frequency" at device tree.
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called of_xilinx_wdt.
-
 # MIPS Architecture

 config ATH79_WDT
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 10/11] watchdog: xilinx: Enable this driver for Zynq

[Michal Simek]

Enable this driver for Zynq.
Move it to architecture independent Kconfig part.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2: None

Build tested by zero day testing system.
---
 drivers/watchdog/Kconfig | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9db5d3c..6120403 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -111,6 +111,15 @@ config WM8350_WATCHDOG
 	  Support for the watchdog in the WM8350 AudioPlus PMIC.  When
 	  the watchdog triggers the system will be reset.

+config XILINX_WATCHDOG
+	tristate "Xilinx Watchdog timer"
+	select WATCHDOG_CORE
+	help
+	  Watchdog driver for the xps_timebase_wdt ip core.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called of_xilinx_wdt.
+
 # ALPHA Architecture

 # ARM Architecture
@@ -1022,19 +1031,6 @@ config M54xx_WATCHDOG

 # MicroBlaze Architecture

-config XILINX_WATCHDOG
-	tristate "Xilinx Watchdog timer"
-	depends on MICROBLAZE
-	select WATCHDOG_CORE
-	---help---
-	  Watchdog driver for the xps_timebase_wdt ip core.
-
-	  IMPORTANT: The xps_timebase_wdt parent must have the property
-	  "clock-frequency" at device tree.
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called of_xilinx_wdt.
-
 # MIPS Architecture

 config ATH79_WDT
--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/3c43ec15/attachment-0001.sig>

[PATCH v3 11/11] watchdog: xilinx: Remove no_timeout variable

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]

Remove no_timeout variable and check variables
directly.

Suggested-by: Rob Herring <robherring2@gmail.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2:
- New patch in this series

 drivers/watchdog/of_xilinx_wdt.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 9dd16cf..c7fb85e 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -147,10 +147,9 @@ static u32 xwdt_selftest(struct xwdt_device *xdev)
 static int xwdt_probe(struct platform_device *pdev)
 {
 	int rc;
-	u32 pfreq, enable_once = 0;
+	u32 pfreq = 0, enable_once = 0;
 	struct resource *res;
 	struct xwdt_device *xdev;
-	bool no_timeout = false;
 	struct watchdog_device *xilinx_wdt_wdd;

 	xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL);
@@ -168,19 +167,15 @@ static int xwdt_probe(struct platform_device *pdev)
 		return PTR_ERR(xdev->base);

 	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
-	if (rc) {
+	if (rc)
 		dev_warn(&pdev->dev,
 			 "The watchdog clock frequency cannot be obtained\n");
-		no_timeout = true;
-	}

 	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
 				  &xdev->wdt_interval);
-	if (rc) {
+	if (rc)
 		dev_warn(&pdev->dev,
 			 "Parameter \"xlnx,wdt-interval\" not found\n");
-		no_timeout = true;
-	}

 	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
 				  &enable_once);
@@ -194,7 +189,7 @@ static int xwdt_probe(struct platform_device *pdev)
 	 * Twice of the 2^wdt_interval / freq  because the first wdt overflow is
 	 * ignored (interrupt), reset is only generated at second wdt overflow
 	 */
-	if (!no_timeout)
+	if (pfreq && xdev->wdt_interval)
 		xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
 					  pfreq);

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 11/11] watchdog: xilinx: Remove no_timeout variable

[Michal Simek]

Remove no_timeout variable and check variables
directly.

Suggested-by: Rob Herring <robherring2@gmail.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

Changes in v3: None
Changes in v2:
- New patch in this series

 drivers/watchdog/of_xilinx_wdt.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index 9dd16cf..c7fb85e 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -147,10 +147,9 @@ static u32 xwdt_selftest(struct xwdt_device *xdev)
 static int xwdt_probe(struct platform_device *pdev)
 {
 	int rc;
-	u32 pfreq, enable_once = 0;
+	u32 pfreq = 0, enable_once = 0;
 	struct resource *res;
 	struct xwdt_device *xdev;
-	bool no_timeout = false;
 	struct watchdog_device *xilinx_wdt_wdd;

 	xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL);
@@ -168,19 +167,15 @@ static int xwdt_probe(struct platform_device *pdev)
 		return PTR_ERR(xdev->base);

 	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
-	if (rc) {
+	if (rc)
 		dev_warn(&pdev->dev,
 			 "The watchdog clock frequency cannot be obtained\n");
-		no_timeout = true;
-	}

 	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
 				  &xdev->wdt_interval);
-	if (rc) {
+	if (rc)
 		dev_warn(&pdev->dev,
 			 "Parameter \"xlnx,wdt-interval\" not found\n");
-		no_timeout = true;
-	}

 	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
 				  &enable_once);
@@ -194,7 +189,7 @@ static int xwdt_probe(struct platform_device *pdev)
 	 * Twice of the 2^wdt_interval / freq  because the first wdt overflow is
 	 * ignored (interrupt), reset is only generated at second wdt overflow
 	 */
-	if (!no_timeout)
+	if (pfreq && xdev->wdt_interval)
 		xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
 					  pfreq);

--
1.8.2.3

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140212/2bdc6693/attachment-0001.sig>

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

Hi Michal,

On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
> Use of_property_read_u32 functions to clean probe function.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 
> Changes in v3:
> - Remove one if checking and use variable directly
> 

Looks good.

Another comment/remark.

> 
> -	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
> -					"clock-frequency", NULL);
> -
> -	if (pfreq == NULL) {
> +	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
> +	if (rc) {
>  		dev_warn(&pdev->dev,
>  			 "The watchdog clock frequency cannot be obtained\n");
>  		no_timeout = true;
>  	}
> 
> -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> -					"xlnx,wdt-interval", NULL);
> -	if (tmptr == NULL) {
> +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
> +				  &xdev->wdt_interval);
> +	if (rc) {
>  		dev_warn(&pdev->dev,
>  			 "Parameter \"xlnx,wdt-interval\" not found\n");
>  		no_timeout = true;
> -	} else {
> -		xdev->wdt_interval = *tmptr;
>  	}
> 
> -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> -					"xlnx,wdt-enable-once", NULL);
> -	if (tmptr == NULL) {
> +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
> +				  &enable_once);
> +	if (rc)
>  		dev_warn(&pdev->dev,
>  			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
> -		watchdog_set_nowayout(xilinx_wdt_wdd, true);
> -	}

All the above properties are optional. Is a warning really
warranted in this case ? I usually associate a warning with
something that is wrong, which is not the case here.

I would encourage you to drop those warnings, but that should be
a separate patch.

Thanks,
Guenter

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

Hi Michal,

On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
> Use of_property_read_u32 functions to clean probe function.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 
> Changes in v3:
> - Remove one if checking and use variable directly
> 

Looks good.

Another comment/remark.

> 
> -	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
> -					"clock-frequency", NULL);
> -
> -	if (pfreq == NULL) {
> +	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
> +	if (rc) {
>  		dev_warn(&pdev->dev,
>  			 "The watchdog clock frequency cannot be obtained\n");
>  		no_timeout = true;
>  	}
> 
> -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> -					"xlnx,wdt-interval", NULL);
> -	if (tmptr == NULL) {
> +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
> +				  &xdev->wdt_interval);
> +	if (rc) {
>  		dev_warn(&pdev->dev,
>  			 "Parameter \"xlnx,wdt-interval\" not found\n");
>  		no_timeout = true;
> -	} else {
> -		xdev->wdt_interval = *tmptr;
>  	}
> 
> -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> -					"xlnx,wdt-enable-once", NULL);
> -	if (tmptr == NULL) {
> +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
> +				  &enable_once);
> +	if (rc)
>  		dev_warn(&pdev->dev,
>  			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
> -		watchdog_set_nowayout(xilinx_wdt_wdd, true);
> -	}

All the above properties are optional. Is a warning really
warranted in this case ? I usually associate a warning with
something that is wrong, which is not the case here.

I would encourage you to drop those warnings, but that should be
a separate patch.

Thanks,
Guenter

Re: [PATCH v3 10/11] watchdog: xilinx: Enable this driver for Zynq

[Wim Van Sebroeck]

Hi Michal,

> Enable this driver for Zynq.
> Move it to architecture independent Kconfig part.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
> Build tested by zero day testing system.
> ---
>  drivers/watchdog/Kconfig | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 9db5d3c..6120403 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -111,6 +111,15 @@ config WM8350_WATCHDOG
>  	  Support for the watchdog in the WM8350 AudioPlus PMIC.  When
>  	  the watchdog triggers the system will be reset.
> 
> +config XILINX_WATCHDOG
> +	tristate "Xilinx Watchdog timer"
> +	select WATCHDOG_CORE
> +	help
> +	  Watchdog driver for the xps_timebase_wdt ip core.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called of_xilinx_wdt.
> +
>  # ALPHA Architecture
> 
>  # ARM Architecture
> @@ -1022,19 +1031,6 @@ config M54xx_WATCHDOG
> 
>  # MicroBlaze Architecture
> 
> -config XILINX_WATCHDOG
> -	tristate "Xilinx Watchdog timer"
> -	depends on MICROBLAZE
> -	select WATCHDOG_CORE
> -	---help---
> -	  Watchdog driver for the xps_timebase_wdt ip core.
> -
> -	  IMPORTANT: The xps_timebase_wdt parent must have the property
> -	  "clock-frequency" at device tree.
> -
> -	  To compile this driver as a module, choose M here: the
> -	  module will be called of_xilinx_wdt.
> -
>  # MIPS Architecture
> 
>  config ATH79_WDT
> --
> 1.8.2.3
> 

Are we sure that we don't need an additional dependancy (like depends on HAS_IOMEM) to not have issues with other architectures?

Kind regards,
Wim.

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Wim Van Sebroeck]

Hi All,

> Hi Michal,
> 
> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
> > Use of_property_read_u32 functions to clean probe function.
> > 
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > 
> > Changes in v3:
> > - Remove one if checking and use variable directly
> > 
> 
> Looks good.
> 
> Another comment/remark.
> 
> > 
> > -	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
> > -					"clock-frequency", NULL);
> > -
> > -	if (pfreq == NULL) {
> > +	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
> > +	if (rc) {
> >  		dev_warn(&pdev->dev,
> >  			 "The watchdog clock frequency cannot be obtained\n");
> >  		no_timeout = true;
> >  	}
> > 
> > -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> > -					"xlnx,wdt-interval", NULL);
> > -	if (tmptr == NULL) {
> > +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
> > +				  &xdev->wdt_interval);
> > +	if (rc) {
> >  		dev_warn(&pdev->dev,
> >  			 "Parameter \"xlnx,wdt-interval\" not found\n");
> >  		no_timeout = true;
> > -	} else {
> > -		xdev->wdt_interval = *tmptr;
> >  	}
> > 
> > -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> > -					"xlnx,wdt-enable-once", NULL);
> > -	if (tmptr == NULL) {
> > +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
> > +				  &enable_once);
> > +	if (rc)
> >  		dev_warn(&pdev->dev,
> >  			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
> > -		watchdog_set_nowayout(xilinx_wdt_wdd, true);
> > -	}
> 
> All the above properties are optional. Is a warning really
> warranted in this case ? I usually associate a warning with
> something that is wrong, which is not the case here.
> 
> I would encourage you to drop those warnings, but that should be
> a separate patch.

I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.

Kind regards,
Wim.

Re: [PATCH v3 10/11] watchdog: xilinx: Enable this driver for Zynq

[Guenter Roeck]

On 02/22/2014 10:44 AM, Wim Van Sebroeck wrote:
> Hi Michal,
>
>> Enable this driver for Zynq.
>> Move it to architecture independent Kconfig part.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>> Build tested by zero day testing system.
>> ---
>>   drivers/watchdog/Kconfig | 22 +++++++++-------------
>>   1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 9db5d3c..6120403 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -111,6 +111,15 @@ config WM8350_WATCHDOG
>>   	  Support for the watchdog in the WM8350 AudioPlus PMIC.  When
>>   	  the watchdog triggers the system will be reset.
>>
>> +config XILINX_WATCHDOG
>> +	tristate "Xilinx Watchdog timer"
>> +	select WATCHDOG_CORE
>> +	help
>> +	  Watchdog driver for the xps_timebase_wdt ip core.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called of_xilinx_wdt.
>> +
>>   # ALPHA Architecture
>>
>>   # ARM Architecture
>> @@ -1022,19 +1031,6 @@ config M54xx_WATCHDOG
>>
>>   # MicroBlaze Architecture
>>
>> -config XILINX_WATCHDOG
>> -	tristate "Xilinx Watchdog timer"
>> -	depends on MICROBLAZE
>> -	select WATCHDOG_CORE
>> -	---help---
>> -	  Watchdog driver for the xps_timebase_wdt ip core.
>> -
>> -	  IMPORTANT: The xps_timebase_wdt parent must have the property
>> -	  "clock-frequency" at device tree.
>> -
>> -	  To compile this driver as a module, choose M here: the
>> -	  module will be called of_xilinx_wdt.
>> -
>>   # MIPS Architecture
>>
>>   config ATH79_WDT
>> --
>> 1.8.2.3
>>
>
> Are we sure that we don't need an additional dependancy (like depends on HAS_IOMEM) to not have issues with other architectures?
>

Hi Wim,

Doesn't look like it. I have the series in my build test and did not
see any failures because of it. See http://server.roeck-us.net:8010/builders,
branch watchdog-next.

Which reminds me that I wanted to reparent the branch to 3.14-rc3.
I just did that; you should be able to see build results in a couple
of hours.

Guenter

[PATCH v3 10/11] watchdog: xilinx: Enable this driver for Zynq

[Guenter Roeck]

On 02/22/2014 10:44 AM, Wim Van Sebroeck wrote:
> Hi Michal,
>
>> Enable this driver for Zynq.
>> Move it to architecture independent Kconfig part.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>> Build tested by zero day testing system.
>> ---
>>   drivers/watchdog/Kconfig | 22 +++++++++-------------
>>   1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 9db5d3c..6120403 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -111,6 +111,15 @@ config WM8350_WATCHDOG
>>   	  Support for the watchdog in the WM8350 AudioPlus PMIC.  When
>>   	  the watchdog triggers the system will be reset.
>>
>> +config XILINX_WATCHDOG
>> +	tristate "Xilinx Watchdog timer"
>> +	select WATCHDOG_CORE
>> +	help
>> +	  Watchdog driver for the xps_timebase_wdt ip core.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called of_xilinx_wdt.
>> +
>>   # ALPHA Architecture
>>
>>   # ARM Architecture
>> @@ -1022,19 +1031,6 @@ config M54xx_WATCHDOG
>>
>>   # MicroBlaze Architecture
>>
>> -config XILINX_WATCHDOG
>> -	tristate "Xilinx Watchdog timer"
>> -	depends on MICROBLAZE
>> -	select WATCHDOG_CORE
>> -	---help---
>> -	  Watchdog driver for the xps_timebase_wdt ip core.
>> -
>> -	  IMPORTANT: The xps_timebase_wdt parent must have the property
>> -	  "clock-frequency" at device tree.
>> -
>> -	  To compile this driver as a module, choose M here: the
>> -	  module will be called of_xilinx_wdt.
>> -
>>   # MIPS Architecture
>>
>>   config ATH79_WDT
>> --
>> 1.8.2.3
>>
>
> Are we sure that we don't need an additional dependancy (like depends on HAS_IOMEM) to not have issues with other architectures?
>

Hi Wim,

Doesn't look like it. I have the series in my build test and did not
see any failures because of it. See http://server.roeck-us.net:8010/builders,
branch watchdog-next.

Which reminds me that I wanted to reparent the branch to 3.14-rc3.
I just did that; you should be able to see build results in a couple
of hours.

Guenter

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>> Hi All,
>>
>>> Hi Michal,
>>>
>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>> Use of_property_read_u32 functions to clean probe function.
>>>>
>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - Remove one if checking and use variable directly
>>>>
>>> Looks good.
>>>
>>> Another comment/remark.
>>>
>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>> -                    "clock-frequency", NULL);
>>>> -
>>>> -    if (pfreq == NULL) {
>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>> +    if (rc) {
>>>>           dev_warn(&pdev->dev,
>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>           no_timeout = true;
>>>>       }
>>>>
>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>> -                    "xlnx,wdt-interval", NULL);
>>>> -    if (tmptr == NULL) {
>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>> +                &xdev->wdt_interval);
>>>> +    if (rc) {
>>>>           dev_warn(&pdev->dev,
>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>           no_timeout = true;
>>>> -    } else {
>>>> -        xdev->wdt_interval = *tmptr;
>>>>       }
>>>>
>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>> -    if (tmptr == NULL) {
>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>> +                &enable_once);
>>>> +    if (rc)
>>>>           dev_warn(&pdev->dev,
>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>> -    }
>>> All the above properties are optional. Is a warning really
>>> warranted in this case ? I usually associate a warning with
>>> something that is wrong, which is not the case here.
>>>
>>> I would encourage you to drop those warnings, but that should be
>>> a separate patch.
>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
> Hi
>
> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>

The dt documentation states that the properties are optional.

Optional properties:
- clock-frequency       : Frequency of clock in Hz
- xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
                           1 - Watchdog can be enabled just once
- xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
                           <val> is integer from 8 to 31.

This clearly conflicts with your statement. An optional property
is just that, optional. If it warrants a warning, it must
not be optional. If you claim that not providing the properties
would be "wrong", why are they defined as optional ?
What is your definition of "wrong" and "must have" ?
How do you expect anyone to know that omitting those
"optional" properties is by some definition "wrong" ?

Guenter

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>> Hi All,
>>
>>> Hi Michal,
>>>
>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>> Use of_property_read_u32 functions to clean probe function.
>>>>
>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - Remove one if checking and use variable directly
>>>>
>>> Looks good.
>>>
>>> Another comment/remark.
>>>
>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>> -                    "clock-frequency", NULL);
>>>> -
>>>> -    if (pfreq == NULL) {
>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>> +    if (rc) {
>>>>           dev_warn(&pdev->dev,
>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>           no_timeout = true;
>>>>       }
>>>>
>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>> -                    "xlnx,wdt-interval", NULL);
>>>> -    if (tmptr == NULL) {
>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>> +                &xdev->wdt_interval);
>>>> +    if (rc) {
>>>>           dev_warn(&pdev->dev,
>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>           no_timeout = true;
>>>> -    } else {
>>>> -        xdev->wdt_interval = *tmptr;
>>>>       }
>>>>
>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>> -    if (tmptr == NULL) {
>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>> +                &enable_once);
>>>> +    if (rc)
>>>>           dev_warn(&pdev->dev,
>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>> -    }
>>> All the above properties are optional. Is a warning really
>>> warranted in this case ? I usually associate a warning with
>>> something that is wrong, which is not the case here.
>>>
>>> I would encourage you to drop those warnings, but that should be
>>> a separate patch.
>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
> Hi
>
> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>

The dt documentation states that the properties are optional.

Optional properties:
- clock-frequency       : Frequency of clock in Hz
- xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
                           1 - Watchdog can be enabled just once
- xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
                           <val> is integer from 8 to 31.

This clearly conflicts with your statement. An optional property
is just that, optional. If it warrants a warning, it must
not be optional. If you claim that not providing the properties
would be "wrong", why are they defined as optional ?
What is your definition of "wrong" and "must have" ?
How do you expect anyone to know that omitting those
"optional" properties is by some definition "wrong" ?

Guenter

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
> Hi All,
>
>> Hi Michal,
>>
>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>> Use of_property_read_u32 functions to clean probe function.
>>>
>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>> ---
>>>
>>> Changes in v3:
>>> - Remove one if checking and use variable directly
>>>
>> Looks good.
>>
>> Another comment/remark.
>>
>>> -	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>> -					"clock-frequency", NULL);
>>> -
>>> -	if (pfreq == NULL) {
>>> +	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>> +	if (rc) {
>>>   		dev_warn(&pdev->dev,
>>>   			 "The watchdog clock frequency cannot be obtained\n");
>>>   		no_timeout = true;
>>>   	}
>>>
>>> -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>> -					"xlnx,wdt-interval", NULL);
>>> -	if (tmptr == NULL) {
>>> +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>> +				&xdev->wdt_interval);
>>> +	if (rc) {
>>>   		dev_warn(&pdev->dev,
>>>   			 "Parameter \"xlnx,wdt-interval\" not found\n");
>>>   		no_timeout = true;
>>> -	} else {
>>> -		xdev->wdt_interval = *tmptr;
>>>   	}
>>>
>>> -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>> -					"xlnx,wdt-enable-once", NULL);
>>> -	if (tmptr == NULL) {
>>> +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>> +				&enable_once);
>>> +	if (rc)
>>>   		dev_warn(&pdev->dev,
>>>   			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>> -		watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>> -	}
>> All the above properties are optional. Is a warning really
>> warranted in this case ? I usually associate a warning with
>> something that is wrong, which is not the case here.
>>
>> I would encourage you to drop those warnings, but that should be
>> a separate patch.
> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
Hi

I support Michal intention, I think it is a warning because device tree 
blob must have the "xlnx,wdt-enable-once" property specified in order to 
allow the system to be sure of the real value of this property. In 
addition to, this warning can be helpful to detect a wrong device tree 
specification.

Best regards
Alejandro



50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
> Hi All,
>
>> Hi Michal,
>>
>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>> Use of_property_read_u32 functions to clean probe function.
>>>
>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>> ---
>>>
>>> Changes in v3:
>>> - Remove one if checking and use variable directly
>>>
>> Looks good.
>>
>> Another comment/remark.
>>
>>> -	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>> -					"clock-frequency", NULL);
>>> -
>>> -	if (pfreq == NULL) {
>>> +	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>> +	if (rc) {
>>>   		dev_warn(&pdev->dev,
>>>   			 "The watchdog clock frequency cannot be obtained\n");
>>>   		no_timeout = true;
>>>   	}
>>>
>>> -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>> -					"xlnx,wdt-interval", NULL);
>>> -	if (tmptr == NULL) {
>>> +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>> +				&xdev->wdt_interval);
>>> +	if (rc) {
>>>   		dev_warn(&pdev->dev,
>>>   			 "Parameter \"xlnx,wdt-interval\" not found\n");
>>>   		no_timeout = true;
>>> -	} else {
>>> -		xdev->wdt_interval = *tmptr;
>>>   	}
>>>
>>> -	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>> -					"xlnx,wdt-enable-once", NULL);
>>> -	if (tmptr == NULL) {
>>> +	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>> +				&enable_once);
>>> +	if (rc)
>>>   		dev_warn(&pdev->dev,
>>>   			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>> -		watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>> -	}
>> All the above properties are optional. Is a warning really
>> warranted in this case ? I usually associate a warning with
>> something that is wrong, which is not the case here.
>>
>> I would encourage you to drop those warnings, but that should be
>> a separate patch.
> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
Hi

I support Michal intention, I think it is a warning because device tree 
blob must have the "xlnx,wdt-enable-once" property specified in order to 
allow the system to be sure of the real value of this property. In 
addition to, this warning can be helpful to detect a wrong device tree 
specification.

Best regards
Alejandro



50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>> Hi All,
>>>>
>>>>> Hi Michal,
>>>>>
>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>
>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3:
>>>>>> - Remove one if checking and use variable directly
>>>>>>
>>>>> Looks good.
>>>>>
>>>>> Another comment/remark.
>>>>>
>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>> -                    "clock-frequency", NULL);
>>>>>> -
>>>>>> -    if (pfreq == NULL) {
>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>> +    if (rc) {
>>>>>>           dev_warn(&pdev->dev,
>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>           no_timeout = true;
>>>>>>       }
>>>>>>
>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>> -    if (tmptr == NULL) {
>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>> + &xdev->wdt_interval);
>>>>>> +    if (rc) {
>>>>>>           dev_warn(&pdev->dev,
>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>           no_timeout = true;
>>>>>> -    } else {
>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>       }
>>>>>>
>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>> -    if (tmptr == NULL) {
>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>> + &enable_once);
>>>>>> +    if (rc)
>>>>>>           dev_warn(&pdev->dev,
>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>> -    }
>>>>> All the above properties are optional. Is a warning really
>>>>> warranted in this case ? I usually associate a warning with
>>>>> something that is wrong, which is not the case here.
>>>>>
>>>>> I would encourage you to drop those warnings, but that should be
>>>>> a separate patch.
>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>> Hi
>>>
>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>
>>
>> The dt documentation states that the properties are optional.
>>
>> Optional properties:
>> - clock-frequency       : Frequency of clock in Hz
>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>                           1 - Watchdog can be enabled just once
>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>> <val> is integer from 8 to 31.
>>
>> This clearly conflicts with your statement. An optional property
>> is just that, optional. If it warrants a warning, it must
>> not be optional. If you claim that not providing the properties
>> would be "wrong", why are they defined as optional ?
> Hi Guenter
>
> I didn't know that these properties was classified as optional...
> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
> have these properties defined in theirs MPD files and theirs values can be obtained during the
> hardware specification to device tree conversion.
>> What is your definition of "wrong" and "must have" ?
> what I mean for "must have" is: if these properties can be obtained
> for all xilinx watchdog devices they must be present in the device tree because they allows
> the system (linux/user) to know exactly how a watchdog device is configured.
> Because these properties always can be obtained from hardware design there is no
> reason to leave them out from the device tree. This is why I consider that a device tree without
> these properties should be considered as "wrong" device tree.
>> How do you expect anyone to know that omitting those
>> "optional" properties is by some definition "wrong" ?
> I'm agree with you.
> Maybe these properties shouldn't be optional.
> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
> when a watchdog daemon ask for this property what is the obtained value ?
> Independently of this value, why do not warn the user about this missing property
> when it can always be in the device tree ?
>

Really, this line of argument doesn't make any sense to me.
"xlnx,wdt-enable-once", for example, is a boolean and means
that the watchdog, when enabled, can not be stopped. It defaults
to false, and thus is inherently optional. Making it mandatory
doesn't really add any value.

Similar, the driver choses defaults for the other values.
If that is incorrect, it should not do it.

Really, something is very fishy here. Either the properties are
optional or they are not. If they are optional, the driver should
accept the fact silently. If they are mandatory, the driver should
act accordingly and bail out if the attributes are not there.

You can not have it both ways. I am close to recommending
that Wim should not accept this driver until that is sorted out.

Guenter

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>> Hi All,
>>>>
>>>>> Hi Michal,
>>>>>
>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>
>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3:
>>>>>> - Remove one if checking and use variable directly
>>>>>>
>>>>> Looks good.
>>>>>
>>>>> Another comment/remark.
>>>>>
>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>> -                    "clock-frequency", NULL);
>>>>>> -
>>>>>> -    if (pfreq == NULL) {
>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>> +    if (rc) {
>>>>>>           dev_warn(&pdev->dev,
>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>           no_timeout = true;
>>>>>>       }
>>>>>>
>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>> -    if (tmptr == NULL) {
>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>> + &xdev->wdt_interval);
>>>>>> +    if (rc) {
>>>>>>           dev_warn(&pdev->dev,
>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>           no_timeout = true;
>>>>>> -    } else {
>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>       }
>>>>>>
>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>> -    if (tmptr == NULL) {
>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>> + &enable_once);
>>>>>> +    if (rc)
>>>>>>           dev_warn(&pdev->dev,
>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>> -    }
>>>>> All the above properties are optional. Is a warning really
>>>>> warranted in this case ? I usually associate a warning with
>>>>> something that is wrong, which is not the case here.
>>>>>
>>>>> I would encourage you to drop those warnings, but that should be
>>>>> a separate patch.
>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>> Hi
>>>
>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>
>>
>> The dt documentation states that the properties are optional.
>>
>> Optional properties:
>> - clock-frequency       : Frequency of clock in Hz
>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>                           1 - Watchdog can be enabled just once
>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>> <val> is integer from 8 to 31.
>>
>> This clearly conflicts with your statement. An optional property
>> is just that, optional. If it warrants a warning, it must
>> not be optional. If you claim that not providing the properties
>> would be "wrong", why are they defined as optional ?
> Hi Guenter
>
> I didn't know that these properties was classified as optional...
> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
> have these properties defined in theirs MPD files and theirs values can be obtained during the
> hardware specification to device tree conversion.
>> What is your definition of "wrong" and "must have" ?
> what I mean for "must have" is: if these properties can be obtained
> for all xilinx watchdog devices they must be present in the device tree because they allows
> the system (linux/user) to know exactly how a watchdog device is configured.
> Because these properties always can be obtained from hardware design there is no
> reason to leave them out from the device tree. This is why I consider that a device tree without
> these properties should be considered as "wrong" device tree.
>> How do you expect anyone to know that omitting those
>> "optional" properties is by some definition "wrong" ?
> I'm agree with you.
> Maybe these properties shouldn't be optional.
> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
> when a watchdog daemon ask for this property what is the obtained value ?
> Independently of this value, why do not warn the user about this missing property
> when it can always be in the device tree ?
>

Really, this line of argument doesn't make any sense to me.
"xlnx,wdt-enable-once", for example, is a boolean and means
that the watchdog, when enabled, can not be stopped. It defaults
to false, and thus is inherently optional. Making it mandatory
doesn't really add any value.

Similar, the driver choses defaults for the other values.
If that is incorrect, it should not do it.

Really, something is very fishy here. Either the properties are
optional or they are not. If they are optional, the driver should
accept the fact silently. If they are mandatory, the driver should
act accordingly and bail out if the attributes are not there.

You can not have it both ways. I am close to recommending
that Wim should not accept this driver until that is sorted out.

Guenter

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>> Hi All,
>>>>>>
>>>>>>> Hi Michal,
>>>>>>>
>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>
>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v3:
>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>
>>>>>>> Looks good.
>>>>>>>
>>>>>>> Another comment/remark.
>>>>>>>
>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>> -
>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>>>> +    if (rc) {
>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>>>           no_timeout = true;
>>>>>>>>       }
>>>>>>>>
>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>>>> + &xdev->wdt_interval);
>>>>>>>> +    if (rc) {
>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>           no_timeout = true;
>>>>>>>> -    } else {
>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>       }
>>>>>>>>
>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>>>> + &enable_once);
>>>>>>>> +    if (rc)
>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>> -    }
>>>>>>> All the above properties are optional. Is a warning really
>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>> something that is wrong, which is not the case here.
>>>>>>>
>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>> a separate patch.
>>>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>>>> Hi
>>>>>
>>>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>>>
>>>>
>>>> The dt documentation states that the properties are optional.
>>>>
>>>> Optional properties:
>>>> - clock-frequency       : Frequency of clock in Hz
>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>                           1 - Watchdog can be enabled just once
>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>>>> <val> is integer from 8 to 31.
>>>>
>>>> This clearly conflicts with your statement. An optional property
>>>> is just that, optional. If it warrants a warning, it must
>>>> not be optional. If you claim that not providing the properties
>>>> would be "wrong", why are they defined as optional ?
>>> Hi Guenter
>>>
>>> I didn't know that these properties was classified as optional...
>>> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
>>> have these properties defined in theirs MPD files and theirs values can be obtained during the
>>> hardware specification to device tree conversion.
>>>> What is your definition of "wrong" and "must have" ?
>>> what I mean for "must have" is: if these properties can be obtained
>>> for all xilinx watchdog devices they must be present in the device tree because they allows
>>> the system (linux/user) to know exactly how a watchdog device is configured.
>>> Because these properties always can be obtained from hardware design there is no
>>> reason to leave them out from the device tree. This is why I consider that a device tree without
>>> these properties should be considered as "wrong" device tree.
>>>> How do you expect anyone to know that omitting those
>>>> "optional" properties is by some definition "wrong" ?
>>> I'm agree with you.
>>> Maybe these properties shouldn't be optional.
>>> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
>>> when a watchdog daemon ask for this property what is the obtained value ?
>>> Independently of this value, why do not warn the user about this missing property
>>> when it can always be in the device tree ?
>>>
>>
>> Really, this line of argument doesn't make any sense to me.
>> "xlnx,wdt-enable-once", for example, is a boolean and means
>> that the watchdog, when enabled, can not be stopped. It defaults
>> to false, and thus is inherently optional. Making it mandatory
>> doesn't really add any value.
>>
>
> If the device has been configured with wdt-enable-once=true
> and the device tree doesn't have this property then a watchdog daemon
> would see it as "false" because it is the default making the system to misbehave...
> A warning during driver loading could help user to identify the problem.
>

All this would give you is a false sense of safety. The property could
just as well be there and be wrong (eg be configured as = <0> when it
should be 1, or with a wrong frequency. Following your logic, every driver
would need to warn about everything, just to be sure.

Guenter

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>> Hi All,
>>>>>>
>>>>>>> Hi Michal,
>>>>>>>
>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>
>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v3:
>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>
>>>>>>> Looks good.
>>>>>>>
>>>>>>> Another comment/remark.
>>>>>>>
>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>> -
>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>>>> +    if (rc) {
>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>>>           no_timeout = true;
>>>>>>>>       }
>>>>>>>>
>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>>>> + &xdev->wdt_interval);
>>>>>>>> +    if (rc) {
>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>           no_timeout = true;
>>>>>>>> -    } else {
>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>       }
>>>>>>>>
>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>>>> + &enable_once);
>>>>>>>> +    if (rc)
>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>> -    }
>>>>>>> All the above properties are optional. Is a warning really
>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>> something that is wrong, which is not the case here.
>>>>>>>
>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>> a separate patch.
>>>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>>>> Hi
>>>>>
>>>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>>>
>>>>
>>>> The dt documentation states that the properties are optional.
>>>>
>>>> Optional properties:
>>>> - clock-frequency       : Frequency of clock in Hz
>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>                           1 - Watchdog can be enabled just once
>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>>>> <val> is integer from 8 to 31.
>>>>
>>>> This clearly conflicts with your statement. An optional property
>>>> is just that, optional. If it warrants a warning, it must
>>>> not be optional. If you claim that not providing the properties
>>>> would be "wrong", why are they defined as optional ?
>>> Hi Guenter
>>>
>>> I didn't know that these properties was classified as optional...
>>> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
>>> have these properties defined in theirs MPD files and theirs values can be obtained during the
>>> hardware specification to device tree conversion.
>>>> What is your definition of "wrong" and "must have" ?
>>> what I mean for "must have" is: if these properties can be obtained
>>> for all xilinx watchdog devices they must be present in the device tree because they allows
>>> the system (linux/user) to know exactly how a watchdog device is configured.
>>> Because these properties always can be obtained from hardware design there is no
>>> reason to leave them out from the device tree. This is why I consider that a device tree without
>>> these properties should be considered as "wrong" device tree.
>>>> How do you expect anyone to know that omitting those
>>>> "optional" properties is by some definition "wrong" ?
>>> I'm agree with you.
>>> Maybe these properties shouldn't be optional.
>>> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
>>> when a watchdog daemon ask for this property what is the obtained value ?
>>> Independently of this value, why do not warn the user about this missing property
>>> when it can always be in the device tree ?
>>>
>>
>> Really, this line of argument doesn't make any sense to me.
>> "xlnx,wdt-enable-once", for example, is a boolean and means
>> that the watchdog, when enabled, can not be stopped. It defaults
>> to false, and thus is inherently optional. Making it mandatory
>> doesn't really add any value.
>>
>
> If the device has been configured with wdt-enable-once=true
> and the device tree doesn't have this property then a watchdog daemon
> would see it as "false" because it is the default making the system to misbehave...
> A warning during driver loading could help user to identify the problem.
>

All this would give you is a false sense of safety. The property could
just as well be there and be wrong (eg be configured as = <0> when it
should be 1, or with a wrong frequency. Following your logic, every driver
would need to warn about everything, just to be sure.

Guenter

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 22/2/2014 3:18 PM, Guenter Roeck wrote:
> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>> Hi All,
>>>
>>>> Hi Michal,
>>>>
>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>
>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>> - Remove one if checking and use variable directly
>>>>>
>>>> Looks good.
>>>>
>>>> Another comment/remark.
>>>>
>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>> -                    "clock-frequency", NULL);
>>>>> -
>>>>> -    if (pfreq == NULL) {
>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>> "clock-frequency",&pfreq);
>>>>> +    if (rc) {
>>>>>           dev_warn(&pdev->dev,
>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>           no_timeout = true;
>>>>>       }
>>>>>
>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>> -    if (tmptr == NULL) {
>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>> "xlnx,wdt-interval",
>>>>> + &xdev->wdt_interval);
>>>>> +    if (rc) {
>>>>>           dev_warn(&pdev->dev,
>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>           no_timeout = true;
>>>>> -    } else {
>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>       }
>>>>>
>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>> -    if (tmptr == NULL) {
>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>> "xlnx,wdt-enable-once",
>>>>> + &enable_once);
>>>>> +    if (rc)
>>>>>           dev_warn(&pdev->dev,
>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>> -    }
>>>> All the above properties are optional. Is a warning really
>>>> warranted in this case ? I usually associate a warning with
>>>> something that is wrong, which is not the case here.
>>>>
>>>> I would encourage you to drop those warnings, but that should be
>>>> a separate patch.
>>> I agree with Guenter: these are not really warnings. Seperate patch 
>>> is thus welcome.
>> Hi
>>
>> I support Michal intention, I think it is a warning because device 
>> tree blob must have the "xlnx,wdt-enable-once" property specified in 
>> order to allow the system to be sure of the real value of this 
>> property. In addition to, this warning can be helpful to detect a 
>> wrong device tree specification.
>>
>
> The dt documentation states that the properties are optional.
>
> Optional properties:
> - clock-frequency       : Frequency of clock in Hz
> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>                           1 - Watchdog can be enabled just once
> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock 
> cycles,
> <val> is integer from 8 to 31.
>
> This clearly conflicts with your statement. An optional property
> is just that, optional. If it warrants a warning, it must
> not be optional. If you claim that not providing the properties
> would be "wrong", why are they defined as optional ?
Hi Guenter

I didn't know that these properties was classified as optional...
I think that they should not be, because all xilinx watchog devices (at 
least for microblaze processor)
have these properties defined in theirs MPD files and theirs values can 
be obtained during the
hardware specification to device tree conversion.
> What is your definition of "wrong" and "must have" ?
what I mean for "must have" is: if these properties can be obtained
for all xilinx watchdog devices they must be present in the device tree 
because they allows
the system (linux/user) to know exactly how a watchdog device is configured.
Because these properties always can be obtained from hardware design 
there is no
reason to leave them out from the device tree. This is why I consider 
that a device tree without
these properties should be considered as "wrong" device tree.
> How do you expect anyone to know that omitting those
> "optional" properties is by some definition "wrong" ?
I'm agree with you.
Maybe these properties shouldn't be optional.
For example suppose that "xlnx,wdt-enable-once" is missing in the device 
tree,
when a watchdog daemon ask for this property what is the obtained value ?
Independently of this value, why do not warn the user about this missing 
property
when it can always be in the device tree ?

Regards
Alejandro




50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 22/2/2014 3:18 PM, Guenter Roeck wrote:
> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>> Hi All,
>>>
>>>> Hi Michal,
>>>>
>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>
>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>> - Remove one if checking and use variable directly
>>>>>
>>>> Looks good.
>>>>
>>>> Another comment/remark.
>>>>
>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>> -                    "clock-frequency", NULL);
>>>>> -
>>>>> -    if (pfreq == NULL) {
>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>> "clock-frequency",&pfreq);
>>>>> +    if (rc) {
>>>>>           dev_warn(&pdev->dev,
>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>           no_timeout = true;
>>>>>       }
>>>>>
>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>> -    if (tmptr == NULL) {
>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>> "xlnx,wdt-interval",
>>>>> + &xdev->wdt_interval);
>>>>> +    if (rc) {
>>>>>           dev_warn(&pdev->dev,
>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>           no_timeout = true;
>>>>> -    } else {
>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>       }
>>>>>
>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>> -    if (tmptr == NULL) {
>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>> "xlnx,wdt-enable-once",
>>>>> + &enable_once);
>>>>> +    if (rc)
>>>>>           dev_warn(&pdev->dev,
>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>> -    }
>>>> All the above properties are optional. Is a warning really
>>>> warranted in this case ? I usually associate a warning with
>>>> something that is wrong, which is not the case here.
>>>>
>>>> I would encourage you to drop those warnings, but that should be
>>>> a separate patch.
>>> I agree with Guenter: these are not really warnings. Seperate patch 
>>> is thus welcome.
>> Hi
>>
>> I support Michal intention, I think it is a warning because device 
>> tree blob must have the "xlnx,wdt-enable-once" property specified in 
>> order to allow the system to be sure of the real value of this 
>> property. In addition to, this warning can be helpful to detect a 
>> wrong device tree specification.
>>
>
> The dt documentation states that the properties are optional.
>
> Optional properties:
> - clock-frequency       : Frequency of clock in Hz
> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>                           1 - Watchdog can be enabled just once
> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock 
> cycles,
> <val> is integer from 8 to 31.
>
> This clearly conflicts with your statement. An optional property
> is just that, optional. If it warrants a warning, it must
> not be optional. If you claim that not providing the properties
> would be "wrong", why are they defined as optional ?
Hi Guenter

I didn't know that these properties was classified as optional...
I think that they should not be, because all xilinx watchog devices (at 
least for microblaze processor)
have these properties defined in theirs MPD files and theirs values can 
be obtained during the
hardware specification to device tree conversion.
> What is your definition of "wrong" and "must have" ?
what I mean for "must have" is: if these properties can be obtained
for all xilinx watchdog devices they must be present in the device tree 
because they allows
the system (linux/user) to know exactly how a watchdog device is configured.
Because these properties always can be obtained from hardware design 
there is no
reason to leave them out from the device tree. This is why I consider 
that a device tree without
these properties should be considered as "wrong" device tree.
> How do you expect anyone to know that omitting those
> "optional" properties is by some definition "wrong" ?
I'm agree with you.
Maybe these properties shouldn't be optional.
For example suppose that "xlnx,wdt-enable-once" is missing in the device 
tree,
when a watchdog daemon ask for this property what is the obtained value ?
Independently of this value, why do not warn the user about this missing 
property
when it can always be in the device tree ?

Regards
Alejandro




50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 22/2/2014 5:36 PM, Guenter Roeck wrote:
> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>> Hi All,
>>>>>
>>>>>> Hi Michal,
>>>>>>
>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>
>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>> - Remove one if checking and use variable directly
>>>>>>>
>>>>>> Looks good.
>>>>>>
>>>>>> Another comment/remark.
>>>>>>
>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>> -                    "clock-frequency", NULL);
>>>>>>> -
>>>>>>> -    if (pfreq == NULL) {
>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>> "clock-frequency",&pfreq);
>>>>>>> +    if (rc) {
>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>                "The watchdog clock frequency cannot be 
>>>>>>> obtained\n");
>>>>>>>           no_timeout = true;
>>>>>>>       }
>>>>>>>
>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>> -    if (tmptr == NULL) {
>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>> "xlnx,wdt-interval",
>>>>>>> + &xdev->wdt_interval);
>>>>>>> +    if (rc) {
>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>           no_timeout = true;
>>>>>>> -    } else {
>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>       }
>>>>>>>
>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>> -    if (tmptr == NULL) {
>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>> "xlnx,wdt-enable-once",
>>>>>>> + &enable_once);
>>>>>>> +    if (rc)
>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>> -    }
>>>>>> All the above properties are optional. Is a warning really
>>>>>> warranted in this case ? I usually associate a warning with
>>>>>> something that is wrong, which is not the case here.
>>>>>>
>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>> a separate patch.
>>>>> I agree with Guenter: these are not really warnings. Seperate 
>>>>> patch is thus welcome.
>>>> Hi
>>>>
>>>> I support Michal intention, I think it is a warning because device 
>>>> tree blob must have the "xlnx,wdt-enable-once" property specified 
>>>> in order to allow the system to be sure of the real value of this 
>>>> property. In addition to, this warning can be helpful to detect a 
>>>> wrong device tree specification.
>>>>
>>>
>>> The dt documentation states that the properties are optional.
>>>
>>> Optional properties:
>>> - clock-frequency       : Frequency of clock in Hz
>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>                           1 - Watchdog can be enabled just once
>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock 
>>> cycles,
>>> <val> is integer from 8 to 31.
>>>
>>> This clearly conflicts with your statement. An optional property
>>> is just that, optional. If it warrants a warning, it must
>>> not be optional. If you claim that not providing the properties
>>> would be "wrong", why are they defined as optional ?
>> Hi Guenter
>>
>> I didn't know that these properties was classified as optional...
>> I think that they should not be, because all xilinx watchog devices 
>> (at least for microblaze processor)
>> have these properties defined in theirs MPD files and theirs values 
>> can be obtained during the
>> hardware specification to device tree conversion.
>>> What is your definition of "wrong" and "must have" ?
>> what I mean for "must have" is: if these properties can be obtained
>> for all xilinx watchdog devices they must be present in the device 
>> tree because they allows
>> the system (linux/user) to know exactly how a watchdog device is 
>> configured.
>> Because these properties always can be obtained from hardware design 
>> there is no
>> reason to leave them out from the device tree. This is why I consider 
>> that a device tree without
>> these properties should be considered as "wrong" device tree.
>>> How do you expect anyone to know that omitting those
>>> "optional" properties is by some definition "wrong" ?
>> I'm agree with you.
>> Maybe these properties shouldn't be optional.
>> For example suppose that "xlnx,wdt-enable-once" is missing in the 
>> device tree,
>> when a watchdog daemon ask for this property what is the obtained 
>> value ?
>> Independently of this value, why do not warn the user about this 
>> missing property
>> when it can always be in the device tree ?
>>
>
> Really, this line of argument doesn't make any sense to me.
> "xlnx,wdt-enable-once", for example, is a boolean and means
> that the watchdog, when enabled, can not be stopped. It defaults
> to false, and thus is inherently optional. Making it mandatory
> doesn't really add any value.
>

If the device has been configured with wdt-enable-once=true
and the device tree doesn't have this property then a watchdog daemon
would see it as "false" because it is the default making the system to 
misbehave...
A warning during driver loading could help user to identify the problem.

Making these properties as mandatory and warn where any of them
are not in device tree may be helpful.

Alejandro



50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 22/2/2014 5:36 PM, Guenter Roeck wrote:
> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>> Hi All,
>>>>>
>>>>>> Hi Michal,
>>>>>>
>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>
>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>> - Remove one if checking and use variable directly
>>>>>>>
>>>>>> Looks good.
>>>>>>
>>>>>> Another comment/remark.
>>>>>>
>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>> -                    "clock-frequency", NULL);
>>>>>>> -
>>>>>>> -    if (pfreq == NULL) {
>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>> "clock-frequency",&pfreq);
>>>>>>> +    if (rc) {
>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>                "The watchdog clock frequency cannot be 
>>>>>>> obtained\n");
>>>>>>>           no_timeout = true;
>>>>>>>       }
>>>>>>>
>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>> -    if (tmptr == NULL) {
>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>> "xlnx,wdt-interval",
>>>>>>> + &xdev->wdt_interval);
>>>>>>> +    if (rc) {
>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>           no_timeout = true;
>>>>>>> -    } else {
>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>       }
>>>>>>>
>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>> -    if (tmptr == NULL) {
>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>> "xlnx,wdt-enable-once",
>>>>>>> + &enable_once);
>>>>>>> +    if (rc)
>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>> -    }
>>>>>> All the above properties are optional. Is a warning really
>>>>>> warranted in this case ? I usually associate a warning with
>>>>>> something that is wrong, which is not the case here.
>>>>>>
>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>> a separate patch.
>>>>> I agree with Guenter: these are not really warnings. Seperate 
>>>>> patch is thus welcome.
>>>> Hi
>>>>
>>>> I support Michal intention, I think it is a warning because device 
>>>> tree blob must have the "xlnx,wdt-enable-once" property specified 
>>>> in order to allow the system to be sure of the real value of this 
>>>> property. In addition to, this warning can be helpful to detect a 
>>>> wrong device tree specification.
>>>>
>>>
>>> The dt documentation states that the properties are optional.
>>>
>>> Optional properties:
>>> - clock-frequency       : Frequency of clock in Hz
>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>                           1 - Watchdog can be enabled just once
>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock 
>>> cycles,
>>> <val> is integer from 8 to 31.
>>>
>>> This clearly conflicts with your statement. An optional property
>>> is just that, optional. If it warrants a warning, it must
>>> not be optional. If you claim that not providing the properties
>>> would be "wrong", why are they defined as optional ?
>> Hi Guenter
>>
>> I didn't know that these properties was classified as optional...
>> I think that they should not be, because all xilinx watchog devices 
>> (at least for microblaze processor)
>> have these properties defined in theirs MPD files and theirs values 
>> can be obtained during the
>> hardware specification to device tree conversion.
>>> What is your definition of "wrong" and "must have" ?
>> what I mean for "must have" is: if these properties can be obtained
>> for all xilinx watchdog devices they must be present in the device 
>> tree because they allows
>> the system (linux/user) to know exactly how a watchdog device is 
>> configured.
>> Because these properties always can be obtained from hardware design 
>> there is no
>> reason to leave them out from the device tree. This is why I consider 
>> that a device tree without
>> these properties should be considered as "wrong" device tree.
>>> How do you expect anyone to know that omitting those
>>> "optional" properties is by some definition "wrong" ?
>> I'm agree with you.
>> Maybe these properties shouldn't be optional.
>> For example suppose that "xlnx,wdt-enable-once" is missing in the 
>> device tree,
>> when a watchdog daemon ask for this property what is the obtained 
>> value ?
>> Independently of this value, why do not warn the user about this 
>> missing property
>> when it can always be in the device tree ?
>>
>
> Really, this line of argument doesn't make any sense to me.
> "xlnx,wdt-enable-once", for example, is a boolean and means
> that the watchdog, when enabled, can not be stopped. It defaults
> to false, and thus is inherently optional. Making it mandatory
> doesn't really add any value.
>

If the device has been configured with wdt-enable-once=true
and the device tree doesn't have this property then a watchdog daemon
would see it as "false" because it is the default making the system to 
misbehave...
A warning during driver loading could help user to identify the problem.

Making these properties as mandatory and warn where any of them
are not in device tree may be helpful.

Alejandro



50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>>> Hi Michal,
>>>>>>>>>
>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Changes in v3:
>>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>>
>>>>>>>>> Looks good.
>>>>>>>>>
>>>>>>>>> Another comment/remark.
>>>>>>>>>
>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>>> -
>>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>>>>>> +    if (rc) {
>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>       }
>>>>>>>>>>
>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>>> +    if (rc) {
>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>>           no_timeout = true;
>>>>>>>>>> -    } else {
>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>>       }
>>>>>>>>>>
>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>>>>>> + &enable_once);
>>>>>>>>>> +    if (rc)
>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>>> -    }
>>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>>
>>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>>> a separate patch.
>>>>>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>>>>>> Hi
>>>>>>>
>>>>>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>>>>>
>>>>>>
>>>>>> The dt documentation states that the properties are optional.
>>>>>>
>>>>>> Optional properties:
>>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>>                           1 - Watchdog can be enabled just once
>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>>>>>> <val> is integer from 8 to 31.
>>>>>>
>>>>>> This clearly conflicts with your statement. An optional property
>>>>>> is just that, optional. If it warrants a warning, it must
>>>>>> not be optional. If you claim that not providing the properties
>>>>>> would be "wrong", why are they defined as optional ?
>>>>> Hi Guenter
>>>>>
>>>>> I didn't know that these properties was classified as optional...
>>>>> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
>>>>> have these properties defined in theirs MPD files and theirs values can be obtained during the
>>>>> hardware specification to device tree conversion.
>>>>>> What is your definition of "wrong" and "must have" ?
>>>>> what I mean for "must have" is: if these properties can be obtained
>>>>> for all xilinx watchdog devices they must be present in the device tree because they allows
>>>>> the system (linux/user) to know exactly how a watchdog device is configured.
>>>>> Because these properties always can be obtained from hardware design there is no
>>>>> reason to leave them out from the device tree. This is why I consider that a device tree without
>>>>> these properties should be considered as "wrong" device tree.
>>>>>> How do you expect anyone to know that omitting those
>>>>>> "optional" properties is by some definition "wrong" ?
>>>>> I'm agree with you.
>>>>> Maybe these properties shouldn't be optional.
>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
>>>>> when a watchdog daemon ask for this property what is the obtained value ?
>>>>> Independently of this value, why do not warn the user about this missing property
>>>>> when it can always be in the device tree ?
>>>>>
>>>>
>>>> Really, this line of argument doesn't make any sense to me.
>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>>> that the watchdog, when enabled, can not be stopped. It defaults
>>>> to false, and thus is inherently optional. Making it mandatory
>>>> doesn't really add any value.
>>>>
>>>
>>> If the device has been configured with wdt-enable-once=true
>>> and the device tree doesn't have this property then a watchdog daemon
>>> would see it as "false" because it is the default making the system to misbehave...
>>> A warning during driver loading could help user to identify the problem.
>>>
>>
>> All this would give you is a false sense of safety. The property could
>> just as well be there and be wrong (eg be configured as = <0> when it
>> should be 1, or with a wrong frequency.
> These issues "cannot" be detected but the missing properties yes.
>> Following your logic, every driver
>> would need to warn about everything, just to be sure.
> Every driver should warn about anything that it considers weird and let the user to decide if it matters or not.
> I think that an example of weird could be the lack of an expected property.
>

I don't think it makes sense to continue this discussion.
We have fundamental differences in opinion which we won't
resolve by repeating our arguments over and over.

Wim, I'll let you decide how to handle this. My recommendation
is to request the author to decide if the properties are optional
or not before accepting this patch set. Either the properties
are optional, and there should be no warnings, or they are
mandatory and the driver should bail out if they are missing.

Guenter

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Guenter Roeck]

On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>>> Hi Michal,
>>>>>>>>>
>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Changes in v3:
>>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>>
>>>>>>>>> Looks good.
>>>>>>>>>
>>>>>>>>> Another comment/remark.
>>>>>>>>>
>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>>> -
>>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>>>>>> +    if (rc) {
>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>       }
>>>>>>>>>>
>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>>> +    if (rc) {
>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>>           no_timeout = true;
>>>>>>>>>> -    } else {
>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>>       }
>>>>>>>>>>
>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>>>>>> + &enable_once);
>>>>>>>>>> +    if (rc)
>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>>> -    }
>>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>>
>>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>>> a separate patch.
>>>>>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>>>>>> Hi
>>>>>>>
>>>>>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>>>>>
>>>>>>
>>>>>> The dt documentation states that the properties are optional.
>>>>>>
>>>>>> Optional properties:
>>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>>                           1 - Watchdog can be enabled just once
>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>>>>>> <val> is integer from 8 to 31.
>>>>>>
>>>>>> This clearly conflicts with your statement. An optional property
>>>>>> is just that, optional. If it warrants a warning, it must
>>>>>> not be optional. If you claim that not providing the properties
>>>>>> would be "wrong", why are they defined as optional ?
>>>>> Hi Guenter
>>>>>
>>>>> I didn't know that these properties was classified as optional...
>>>>> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
>>>>> have these properties defined in theirs MPD files and theirs values can be obtained during the
>>>>> hardware specification to device tree conversion.
>>>>>> What is your definition of "wrong" and "must have" ?
>>>>> what I mean for "must have" is: if these properties can be obtained
>>>>> for all xilinx watchdog devices they must be present in the device tree because they allows
>>>>> the system (linux/user) to know exactly how a watchdog device is configured.
>>>>> Because these properties always can be obtained from hardware design there is no
>>>>> reason to leave them out from the device tree. This is why I consider that a device tree without
>>>>> these properties should be considered as "wrong" device tree.
>>>>>> How do you expect anyone to know that omitting those
>>>>>> "optional" properties is by some definition "wrong" ?
>>>>> I'm agree with you.
>>>>> Maybe these properties shouldn't be optional.
>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
>>>>> when a watchdog daemon ask for this property what is the obtained value ?
>>>>> Independently of this value, why do not warn the user about this missing property
>>>>> when it can always be in the device tree ?
>>>>>
>>>>
>>>> Really, this line of argument doesn't make any sense to me.
>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>>> that the watchdog, when enabled, can not be stopped. It defaults
>>>> to false, and thus is inherently optional. Making it mandatory
>>>> doesn't really add any value.
>>>>
>>>
>>> If the device has been configured with wdt-enable-once=true
>>> and the device tree doesn't have this property then a watchdog daemon
>>> would see it as "false" because it is the default making the system to misbehave...
>>> A warning during driver loading could help user to identify the problem.
>>>
>>
>> All this would give you is a false sense of safety. The property could
>> just as well be there and be wrong (eg be configured as = <0> when it
>> should be 1, or with a wrong frequency.
> These issues "cannot" be detected but the missing properties yes.
>> Following your logic, every driver
>> would need to warn about everything, just to be sure.
> Every driver should warn about anything that it considers weird and let the user to decide if it matters or not.
> I think that an example of weird could be the lack of an expected property.
>

I don't think it makes sense to continue this discussion.
We have fundamental differences in opinion which we won't
resolve by repeating our arguments over and over.

Wim, I'll let you decide how to handle this. My recommendation
is to request the author to decide if the properties are optional
or not before accepting this patch set. Either the properties
are optional, and there should be no warnings, or they are
mandatory and the driver should bail out if they are missing.

Guenter

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 22/2/2014 7:44 PM, Guenter Roeck wrote:
> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>> Hi All,
>>>>>>>
>>>>>>>> Hi Michal,
>>>>>>>>
>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Changes in v3:
>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>
>>>>>>>> Looks good.
>>>>>>>>
>>>>>>>> Another comment/remark.
>>>>>>>>
>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>> -
>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>> "clock-frequency",&pfreq);
>>>>>>>>> +    if (rc) {
>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>                "The watchdog clock frequency cannot be 
>>>>>>>>> obtained\n");
>>>>>>>>>           no_timeout = true;
>>>>>>>>>       }
>>>>>>>>>
>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>> "xlnx,wdt-interval",
>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>> +    if (rc) {
>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>           no_timeout = true;
>>>>>>>>> -    } else {
>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>       }
>>>>>>>>>
>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>> "xlnx,wdt-enable-once",
>>>>>>>>> + &enable_once);
>>>>>>>>> +    if (rc)
>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>> -    }
>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>
>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>> a separate patch.
>>>>>>> I agree with Guenter: these are not really warnings. Seperate 
>>>>>>> patch is thus welcome.
>>>>>> Hi
>>>>>>
>>>>>> I support Michal intention, I think it is a warning because 
>>>>>> device tree blob must have the "xlnx,wdt-enable-once" property 
>>>>>> specified in order to allow the system to be sure of the real 
>>>>>> value of this property. In addition to, this warning can be 
>>>>>> helpful to detect a wrong device tree specification.
>>>>>>
>>>>>
>>>>> The dt documentation states that the properties are optional.
>>>>>
>>>>> Optional properties:
>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>                           1 - Watchdog can be enabled just once
>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> 
>>>>> clock cycles,
>>>>> <val> is integer from 8 to 31.
>>>>>
>>>>> This clearly conflicts with your statement. An optional property
>>>>> is just that, optional. If it warrants a warning, it must
>>>>> not be optional. If you claim that not providing the properties
>>>>> would be "wrong", why are they defined as optional ?
>>>> Hi Guenter
>>>>
>>>> I didn't know that these properties was classified as optional...
>>>> I think that they should not be, because all xilinx watchog devices 
>>>> (at least for microblaze processor)
>>>> have these properties defined in theirs MPD files and theirs values 
>>>> can be obtained during the
>>>> hardware specification to device tree conversion.
>>>>> What is your definition of "wrong" and "must have" ?
>>>> what I mean for "must have" is: if these properties can be obtained
>>>> for all xilinx watchdog devices they must be present in the device 
>>>> tree because they allows
>>>> the system (linux/user) to know exactly how a watchdog device is 
>>>> configured.
>>>> Because these properties always can be obtained from hardware 
>>>> design there is no
>>>> reason to leave them out from the device tree. This is why I 
>>>> consider that a device tree without
>>>> these properties should be considered as "wrong" device tree.
>>>>> How do you expect anyone to know that omitting those
>>>>> "optional" properties is by some definition "wrong" ?
>>>> I'm agree with you.
>>>> Maybe these properties shouldn't be optional.
>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the 
>>>> device tree,
>>>> when a watchdog daemon ask for this property what is the obtained 
>>>> value ?
>>>> Independently of this value, why do not warn the user about this 
>>>> missing property
>>>> when it can always be in the device tree ?
>>>>
>>>
>>> Really, this line of argument doesn't make any sense to me.
>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>> that the watchdog, when enabled, can not be stopped. It defaults
>>> to false, and thus is inherently optional. Making it mandatory
>>> doesn't really add any value.
>>>
>>
>> If the device has been configured with wdt-enable-once=true
>> and the device tree doesn't have this property then a watchdog daemon
>> would see it as "false" because it is the default making the system 
>> to misbehave...
>> A warning during driver loading could help user to identify the problem.
>>
>
> All this would give you is a false sense of safety. The property could
> just as well be there and be wrong (eg be configured as = <0> when it
> should be 1, or with a wrong frequency. 
These issues "cannot" be detected but the missing properties yes.
> Following your logic, every driver
> would need to warn about everything, just to be sure.
Every driver should warn about anything that it considers weird and let 
the user to decide if it matters or not.
I think that an example of weird could be the lack of an expected property.

Alejandro





50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 22/2/2014 7:44 PM, Guenter Roeck wrote:
> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>> Hi All,
>>>>>>>
>>>>>>>> Hi Michal,
>>>>>>>>
>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Changes in v3:
>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>
>>>>>>>> Looks good.
>>>>>>>>
>>>>>>>> Another comment/remark.
>>>>>>>>
>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>> -
>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>> "clock-frequency",&pfreq);
>>>>>>>>> +    if (rc) {
>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>                "The watchdog clock frequency cannot be 
>>>>>>>>> obtained\n");
>>>>>>>>>           no_timeout = true;
>>>>>>>>>       }
>>>>>>>>>
>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>> "xlnx,wdt-interval",
>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>> +    if (rc) {
>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>           no_timeout = true;
>>>>>>>>> -    } else {
>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>       }
>>>>>>>>>
>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>> "xlnx,wdt-enable-once",
>>>>>>>>> + &enable_once);
>>>>>>>>> +    if (rc)
>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>> -    }
>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>
>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>> a separate patch.
>>>>>>> I agree with Guenter: these are not really warnings. Seperate 
>>>>>>> patch is thus welcome.
>>>>>> Hi
>>>>>>
>>>>>> I support Michal intention, I think it is a warning because 
>>>>>> device tree blob must have the "xlnx,wdt-enable-once" property 
>>>>>> specified in order to allow the system to be sure of the real 
>>>>>> value of this property. In addition to, this warning can be 
>>>>>> helpful to detect a wrong device tree specification.
>>>>>>
>>>>>
>>>>> The dt documentation states that the properties are optional.
>>>>>
>>>>> Optional properties:
>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>                           1 - Watchdog can be enabled just once
>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> 
>>>>> clock cycles,
>>>>> <val> is integer from 8 to 31.
>>>>>
>>>>> This clearly conflicts with your statement. An optional property
>>>>> is just that, optional. If it warrants a warning, it must
>>>>> not be optional. If you claim that not providing the properties
>>>>> would be "wrong", why are they defined as optional ?
>>>> Hi Guenter
>>>>
>>>> I didn't know that these properties was classified as optional...
>>>> I think that they should not be, because all xilinx watchog devices 
>>>> (at least for microblaze processor)
>>>> have these properties defined in theirs MPD files and theirs values 
>>>> can be obtained during the
>>>> hardware specification to device tree conversion.
>>>>> What is your definition of "wrong" and "must have" ?
>>>> what I mean for "must have" is: if these properties can be obtained
>>>> for all xilinx watchdog devices they must be present in the device 
>>>> tree because they allows
>>>> the system (linux/user) to know exactly how a watchdog device is 
>>>> configured.
>>>> Because these properties always can be obtained from hardware 
>>>> design there is no
>>>> reason to leave them out from the device tree. This is why I 
>>>> consider that a device tree without
>>>> these properties should be considered as "wrong" device tree.
>>>>> How do you expect anyone to know that omitting those
>>>>> "optional" properties is by some definition "wrong" ?
>>>> I'm agree with you.
>>>> Maybe these properties shouldn't be optional.
>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the 
>>>> device tree,
>>>> when a watchdog daemon ask for this property what is the obtained 
>>>> value ?
>>>> Independently of this value, why do not warn the user about this 
>>>> missing property
>>>> when it can always be in the device tree ?
>>>>
>>>
>>> Really, this line of argument doesn't make any sense to me.
>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>> that the watchdog, when enabled, can not be stopped. It defaults
>>> to false, and thus is inherently optional. Making it mandatory
>>> doesn't really add any value.
>>>
>>
>> If the device has been configured with wdt-enable-once=true
>> and the device tree doesn't have this property then a watchdog daemon
>> would see it as "false" because it is the default making the system 
>> to misbehave...
>> A warning during driver loading could help user to identify the problem.
>>
>
> All this would give you is a false sense of safety. The property could
> just as well be there and be wrong (eg be configured as = <0> when it
> should be 1, or with a wrong frequency. 
These issues "cannot" be detected but the missing properties yes.
> Following your logic, every driver
> would need to warn about everything, just to be sure.
Every driver should warn about anything that it considers weird and let 
the user to decide if it matters or not.
I think that an example of weird could be the lack of an expected property.

Alejandro





50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 23/2/2014 6:43 AM, Guenter Roeck wrote:
> On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
>> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
>>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>>> Hi Michal,
>>>>>>>>>>
>>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Changes in v3:
>>>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>>>
>>>>>>>>>> Looks good.
>>>>>>>>>>
>>>>>>>>>> Another comment/remark.
>>>>>>>>>>
>>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>>>> -
>>>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>>>> "clock-frequency",&pfreq);
>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>                "The watchdog clock frequency cannot be 
>>>>>>>>>>> obtained\n");
>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>       }
>>>>>>>>>>>
>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>>>> "xlnx,wdt-interval",
>>>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>> -    } else {
>>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>>>       }
>>>>>>>>>>>
>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>>>> "xlnx,wdt-enable-once",
>>>>>>>>>>> + &enable_once);
>>>>>>>>>>> +    if (rc)
>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not 
>>>>>>>>>>> found\n");
>>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>>>> -    }
>>>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>>>
>>>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>>>> a separate patch.
>>>>>>>>> I agree with Guenter: these are not really warnings. Seperate 
>>>>>>>>> patch is thus welcome.
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> I support Michal intention, I think it is a warning because 
>>>>>>>> device tree blob must have the "xlnx,wdt-enable-once" property 
>>>>>>>> specified in order to allow the system to be sure of the real 
>>>>>>>> value of this property. In addition to, this warning can be 
>>>>>>>> helpful to detect a wrong device tree specification.
>>>>>>>>
>>>>>>>
>>>>>>> The dt documentation states that the properties are optional.
>>>>>>>
>>>>>>> Optional properties:
>>>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>>>                           1 - Watchdog can be enabled just once
>>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> 
>>>>>>> clock cycles,
>>>>>>> <val> is integer from 8 to 31.
>>>>>>>
>>>>>>> This clearly conflicts with your statement. An optional property
>>>>>>> is just that, optional. If it warrants a warning, it must
>>>>>>> not be optional. If you claim that not providing the properties
>>>>>>> would be "wrong", why are they defined as optional ?
>>>>>> Hi Guenter
>>>>>>
>>>>>> I didn't know that these properties was classified as optional...
>>>>>> I think that they should not be, because all xilinx watchog 
>>>>>> devices (at least for microblaze processor)
>>>>>> have these properties defined in theirs MPD files and theirs 
>>>>>> values can be obtained during the
>>>>>> hardware specification to device tree conversion.
>>>>>>> What is your definition of "wrong" and "must have" ?
>>>>>> what I mean for "must have" is: if these properties can be obtained
>>>>>> for all xilinx watchdog devices they must be present in the 
>>>>>> device tree because they allows
>>>>>> the system (linux/user) to know exactly how a watchdog device is 
>>>>>> configured.
>>>>>> Because these properties always can be obtained from hardware 
>>>>>> design there is no
>>>>>> reason to leave them out from the device tree. This is why I 
>>>>>> consider that a device tree without
>>>>>> these properties should be considered as "wrong" device tree.
>>>>>>> How do you expect anyone to know that omitting those
>>>>>>> "optional" properties is by some definition "wrong" ?
>>>>>> I'm agree with you.
>>>>>> Maybe these properties shouldn't be optional.
>>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the 
>>>>>> device tree,
>>>>>> when a watchdog daemon ask for this property what is the obtained 
>>>>>> value ?
>>>>>> Independently of this value, why do not warn the user about this 
>>>>>> missing property
>>>>>> when it can always be in the device tree ?
>>>>>>
>>>>>
>>>>> Really, this line of argument doesn't make any sense to me.
>>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>>>> that the watchdog, when enabled, can not be stopped. It defaults
>>>>> to false, and thus is inherently optional. Making it mandatory
>>>>> doesn't really add any value.
>>>>>
>>>>
>>>> If the device has been configured with wdt-enable-once=true
>>>> and the device tree doesn't have this property then a watchdog daemon
>>>> would see it as "false" because it is the default making the system 
>>>> to misbehave...
>>>> A warning during driver loading could help user to identify the 
>>>> problem.
>>>>
>>>
>>> All this would give you is a false sense of safety. The property could
>>> just as well be there and be wrong (eg be configured as = <0> when it
>>> should be 1, or with a wrong frequency.
>> These issues "cannot" be detected but the missing properties yes.
>>> Following your logic, every driver
>>> would need to warn about everything, just to be sure.
>> Every driver should warn about anything that it considers weird and 
>> let the user to decide if it matters or not.
>> I think that an example of weird could be the lack of an expected 
>> property.
>>
>
> I don't think it makes sense to continue this discussion.
> We have fundamental differences in opinion which we won't
> resolve by repeating our arguments over and over.
>
> Wim, I'll let you decide how to handle this. My recommendation
> is to request the author to decide if the properties are optional
> or not before accepting this patch set. Either the properties
> are optional, and there should be no warnings, or they are
> mandatory and the driver should bail out if they are missing.
>
I'm totally agreed with you :)




50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Alejandro Cabrera]

On 23/2/2014 6:43 AM, Guenter Roeck wrote:
> On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
>> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
>>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>>> Hi Michal,
>>>>>>>>>>
>>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Changes in v3:
>>>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>>>
>>>>>>>>>> Looks good.
>>>>>>>>>>
>>>>>>>>>> Another comment/remark.
>>>>>>>>>>
>>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>>>> -
>>>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>>>> "clock-frequency",&pfreq);
>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>                "The watchdog clock frequency cannot be 
>>>>>>>>>>> obtained\n");
>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>       }
>>>>>>>>>>>
>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>>>> "xlnx,wdt-interval",
>>>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>> -    } else {
>>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>>>       }
>>>>>>>>>>>
>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, 
>>>>>>>>>>> "xlnx,wdt-enable-once",
>>>>>>>>>>> + &enable_once);
>>>>>>>>>>> +    if (rc)
>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not 
>>>>>>>>>>> found\n");
>>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>>>> -    }
>>>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>>>
>>>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>>>> a separate patch.
>>>>>>>>> I agree with Guenter: these are not really warnings. Seperate 
>>>>>>>>> patch is thus welcome.
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> I support Michal intention, I think it is a warning because 
>>>>>>>> device tree blob must have the "xlnx,wdt-enable-once" property 
>>>>>>>> specified in order to allow the system to be sure of the real 
>>>>>>>> value of this property. In addition to, this warning can be 
>>>>>>>> helpful to detect a wrong device tree specification.
>>>>>>>>
>>>>>>>
>>>>>>> The dt documentation states that the properties are optional.
>>>>>>>
>>>>>>> Optional properties:
>>>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>>>                           1 - Watchdog can be enabled just once
>>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> 
>>>>>>> clock cycles,
>>>>>>> <val> is integer from 8 to 31.
>>>>>>>
>>>>>>> This clearly conflicts with your statement. An optional property
>>>>>>> is just that, optional. If it warrants a warning, it must
>>>>>>> not be optional. If you claim that not providing the properties
>>>>>>> would be "wrong", why are they defined as optional ?
>>>>>> Hi Guenter
>>>>>>
>>>>>> I didn't know that these properties was classified as optional...
>>>>>> I think that they should not be, because all xilinx watchog 
>>>>>> devices (at least for microblaze processor)
>>>>>> have these properties defined in theirs MPD files and theirs 
>>>>>> values can be obtained during the
>>>>>> hardware specification to device tree conversion.
>>>>>>> What is your definition of "wrong" and "must have" ?
>>>>>> what I mean for "must have" is: if these properties can be obtained
>>>>>> for all xilinx watchdog devices they must be present in the 
>>>>>> device tree because they allows
>>>>>> the system (linux/user) to know exactly how a watchdog device is 
>>>>>> configured.
>>>>>> Because these properties always can be obtained from hardware 
>>>>>> design there is no
>>>>>> reason to leave them out from the device tree. This is why I 
>>>>>> consider that a device tree without
>>>>>> these properties should be considered as "wrong" device tree.
>>>>>>> How do you expect anyone to know that omitting those
>>>>>>> "optional" properties is by some definition "wrong" ?
>>>>>> I'm agree with you.
>>>>>> Maybe these properties shouldn't be optional.
>>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the 
>>>>>> device tree,
>>>>>> when a watchdog daemon ask for this property what is the obtained 
>>>>>> value ?
>>>>>> Independently of this value, why do not warn the user about this 
>>>>>> missing property
>>>>>> when it can always be in the device tree ?
>>>>>>
>>>>>
>>>>> Really, this line of argument doesn't make any sense to me.
>>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>>>> that the watchdog, when enabled, can not be stopped. It defaults
>>>>> to false, and thus is inherently optional. Making it mandatory
>>>>> doesn't really add any value.
>>>>>
>>>>
>>>> If the device has been configured with wdt-enable-once=true
>>>> and the device tree doesn't have this property then a watchdog daemon
>>>> would see it as "false" because it is the default making the system 
>>>> to misbehave...
>>>> A warning during driver loading could help user to identify the 
>>>> problem.
>>>>
>>>
>>> All this would give you is a false sense of safety. The property could
>>> just as well be there and be wrong (eg be configured as = <0> when it
>>> should be 1, or with a wrong frequency.
>> These issues "cannot" be detected but the missing properties yes.
>>> Following your logic, every driver
>>> would need to warn about everything, just to be sure.
>> Every driver should warn about anything that it considers weird and 
>> let the user to decide if it matters or not.
>> I think that an example of weird could be the lack of an expected 
>> property.
>>
>
> I don't think it makes sense to continue this discussion.
> We have fundamental differences in opinion which we won't
> resolve by repeating our arguments over and over.
>
> Wim, I'll let you decide how to handle this. My recommendation
> is to request the author to decide if the properties are optional
> or not before accepting this patch set. Either the properties
> are optional, and there should be no warnings, or they are
> mandatory and the driver should bail out if they are missing.
>
I'm totally agreed with you :)




50 Aniversario de la Cujae. Inaugurada por Fidel el 2 de diciembre de 1964  http://cujae.edu.cu

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 8719 bytes --]

On 02/23/2014 08:00 PM, Alejandro Cabrera wrote:
> On 23/2/2014 6:43 AM, Guenter Roeck wrote:
>> On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
>>> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
>>>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>>>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>>>>> Hi All,
>>>>>>>>>>
>>>>>>>>>>> Hi Michal,
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>>>>
>>>>>>>>>>> Looks good.
>>>>>>>>>>>
>>>>>>>>>>> Another comment/remark.
>>>>>>>>>>>
>>>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>>>>> -
>>>>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>>       }
>>>>>>>>>>>>
>>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>> -    } else {
>>>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>>>>       }
>>>>>>>>>>>>
>>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>>>>>>>> + &enable_once);
>>>>>>>>>>>> +    if (rc)
>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>>>>> -    }
>>>>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>>>>
>>>>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>>>>> a separate patch.
>>>>>>>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>>>>>>>
>>>>>>>>
>>>>>>>> The dt documentation states that the properties are optional.
>>>>>>>>
>>>>>>>> Optional properties:
>>>>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>>>>                           1 - Watchdog can be enabled just once
>>>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>>>>>>>> <val> is integer from 8 to 31.
>>>>>>>>
>>>>>>>> This clearly conflicts with your statement. An optional property
>>>>>>>> is just that, optional. If it warrants a warning, it must
>>>>>>>> not be optional. If you claim that not providing the properties
>>>>>>>> would be "wrong", why are they defined as optional ?
>>>>>>> Hi Guenter
>>>>>>>
>>>>>>> I didn't know that these properties was classified as optional...
>>>>>>> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
>>>>>>> have these properties defined in theirs MPD files and theirs values can be obtained during the
>>>>>>> hardware specification to device tree conversion.
>>>>>>>> What is your definition of "wrong" and "must have" ?
>>>>>>> what I mean for "must have" is: if these properties can be obtained
>>>>>>> for all xilinx watchdog devices they must be present in the device tree because they allows
>>>>>>> the system (linux/user) to know exactly how a watchdog device is configured.
>>>>>>> Because these properties always can be obtained from hardware design there is no
>>>>>>> reason to leave them out from the device tree. This is why I consider that a device tree without
>>>>>>> these properties should be considered as "wrong" device tree.
>>>>>>>> How do you expect anyone to know that omitting those
>>>>>>>> "optional" properties is by some definition "wrong" ?
>>>>>>> I'm agree with you.
>>>>>>> Maybe these properties shouldn't be optional.
>>>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
>>>>>>> when a watchdog daemon ask for this property what is the obtained value ?
>>>>>>> Independently of this value, why do not warn the user about this missing property
>>>>>>> when it can always be in the device tree ?
>>>>>>>
>>>>>>
>>>>>> Really, this line of argument doesn't make any sense to me.
>>>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>>>>> that the watchdog, when enabled, can not be stopped. It defaults
>>>>>> to false, and thus is inherently optional. Making it mandatory
>>>>>> doesn't really add any value.
>>>>>>
>>>>>
>>>>> If the device has been configured with wdt-enable-once=true
>>>>> and the device tree doesn't have this property then a watchdog daemon
>>>>> would see it as "false" because it is the default making the system to misbehave...
>>>>> A warning during driver loading could help user to identify the problem.
>>>>>
>>>>
>>>> All this would give you is a false sense of safety. The property could
>>>> just as well be there and be wrong (eg be configured as = <0> when it
>>>> should be 1, or with a wrong frequency.
>>> These issues "cannot" be detected but the missing properties yes.
>>>> Following your logic, every driver
>>>> would need to warn about everything, just to be sure.
>>> Every driver should warn about anything that it considers weird and let the user to decide if it matters or not.
>>> I think that an example of weird could be the lack of an expected property.
>>>
>>
>> I don't think it makes sense to continue this discussion.
>> We have fundamental differences in opinion which we won't
>> resolve by repeating our arguments over and over.
>>
>> Wim, I'll let you decide how to handle this. My recommendation
>> is to request the author to decide if the properties are optional
>> or not before accepting this patch set. Either the properties
>> are optional, and there should be no warnings, or they are
>> mandatory and the driver should bail out if they are missing.
>>
> I'm totally agreed with you :)
>

You have reached to completely different discussion.
We should talk just about the code I have sent.
I have checked what I have done and the intention was just to have better driver.
I even didn't change any logic about DT and probe. DT binding just describes
what it is written in the driver, nothing more nothing else.

Please keep this in your mind.

The driver with this binding is in the kernel for a while and if binding is wrong
let's change it but it should be in separate patch which can fix binding
warn/error messages.

Wim: If you agree, I have no problem to send this follow up patch
which can be applied on the top of this series.
We can make xlnx,wdt-enable-once and xlnx,wdt-interval as required properties
and clock-frequence can go away and we can use CCF.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Michal Simek]

On 02/23/2014 08:00 PM, Alejandro Cabrera wrote:
> On 23/2/2014 6:43 AM, Guenter Roeck wrote:
>> On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
>>> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
>>>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>>>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>>>>> Hi All,
>>>>>>>>>>
>>>>>>>>>>> Hi Michal,
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>>>>
>>>>>>>>>>> Looks good.
>>>>>>>>>>>
>>>>>>>>>>> Another comment/remark.
>>>>>>>>>>>
>>>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>>>>> -
>>>>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>>       }
>>>>>>>>>>>>
>>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>> -    } else {
>>>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>>>>       }
>>>>>>>>>>>>
>>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>>>>>>>> + &enable_once);
>>>>>>>>>>>> +    if (rc)
>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>>>>> -    }
>>>>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>>>>
>>>>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>>>>> a separate patch.
>>>>>>>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>>>>>>>
>>>>>>>>
>>>>>>>> The dt documentation states that the properties are optional.
>>>>>>>>
>>>>>>>> Optional properties:
>>>>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>>>>                           1 - Watchdog can be enabled just once
>>>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>>>>>>>> <val> is integer from 8 to 31.
>>>>>>>>
>>>>>>>> This clearly conflicts with your statement. An optional property
>>>>>>>> is just that, optional. If it warrants a warning, it must
>>>>>>>> not be optional. If you claim that not providing the properties
>>>>>>>> would be "wrong", why are they defined as optional ?
>>>>>>> Hi Guenter
>>>>>>>
>>>>>>> I didn't know that these properties was classified as optional...
>>>>>>> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
>>>>>>> have these properties defined in theirs MPD files and theirs values can be obtained during the
>>>>>>> hardware specification to device tree conversion.
>>>>>>>> What is your definition of "wrong" and "must have" ?
>>>>>>> what I mean for "must have" is: if these properties can be obtained
>>>>>>> for all xilinx watchdog devices they must be present in the device tree because they allows
>>>>>>> the system (linux/user) to know exactly how a watchdog device is configured.
>>>>>>> Because these properties always can be obtained from hardware design there is no
>>>>>>> reason to leave them out from the device tree. This is why I consider that a device tree without
>>>>>>> these properties should be considered as "wrong" device tree.
>>>>>>>> How do you expect anyone to know that omitting those
>>>>>>>> "optional" properties is by some definition "wrong" ?
>>>>>>> I'm agree with you.
>>>>>>> Maybe these properties shouldn't be optional.
>>>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
>>>>>>> when a watchdog daemon ask for this property what is the obtained value ?
>>>>>>> Independently of this value, why do not warn the user about this missing property
>>>>>>> when it can always be in the device tree ?
>>>>>>>
>>>>>>
>>>>>> Really, this line of argument doesn't make any sense to me.
>>>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>>>>> that the watchdog, when enabled, can not be stopped. It defaults
>>>>>> to false, and thus is inherently optional. Making it mandatory
>>>>>> doesn't really add any value.
>>>>>>
>>>>>
>>>>> If the device has been configured with wdt-enable-once=true
>>>>> and the device tree doesn't have this property then a watchdog daemon
>>>>> would see it as "false" because it is the default making the system to misbehave...
>>>>> A warning during driver loading could help user to identify the problem.
>>>>>
>>>>
>>>> All this would give you is a false sense of safety. The property could
>>>> just as well be there and be wrong (eg be configured as = <0> when it
>>>> should be 1, or with a wrong frequency.
>>> These issues "cannot" be detected but the missing properties yes.
>>>> Following your logic, every driver
>>>> would need to warn about everything, just to be sure.
>>> Every driver should warn about anything that it considers weird and let the user to decide if it matters or not.
>>> I think that an example of weird could be the lack of an expected property.
>>>
>>
>> I don't think it makes sense to continue this discussion.
>> We have fundamental differences in opinion which we won't
>> resolve by repeating our arguments over and over.
>>
>> Wim, I'll let you decide how to handle this. My recommendation
>> is to request the author to decide if the properties are optional
>> or not before accepting this patch set. Either the properties
>> are optional, and there should be no warnings, or they are
>> mandatory and the driver should bail out if they are missing.
>>
> I'm totally agreed with you :)
>

You have reached to completely different discussion.
We should talk just about the code I have sent.
I have checked what I have done and the intention was just to have better driver.
I even didn't change any logic about DT and probe. DT binding just describes
what it is written in the driver, nothing more nothing else.

Please keep this in your mind.

The driver with this binding is in the kernel for a while and if binding is wrong
let's change it but it should be in separate patch which can fix binding
warn/error messages.

Wim: If you agree, I have no problem to send this follow up patch
which can be applied on the top of this series.
We can make xlnx,wdt-enable-once and xlnx,wdt-interval as required properties
and clock-frequence can go away and we can use CCF.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140224/b7ec8ba5/attachment-0001.sig>

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Wim Van Sebroeck]

Hi Michal,

> On 02/23/2014 08:00 PM, Alejandro Cabrera wrote:
> > On 23/2/2014 6:43 AM, Guenter Roeck wrote:
> >> On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
> >>> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
> >>>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
> >>>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
> >>>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
> >>>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
> >>>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
> >>>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
> >>>>>>>>>> Hi All,
> >>>>>>>>>>
> >>>>>>>>>>> Hi Michal,
> >>>>>>>>>>>
> >>>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
> >>>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
> >>>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>>
> >>>>>>>>>>>> Changes in v3:
> >>>>>>>>>>>> - Remove one if checking and use variable directly
> >>>>>>>>>>>>
> >>>>>>>>>>> Looks good.
> >>>>>>>>>>>
> >>>>>>>>>>> Another comment/remark.
> >>>>>>>>>>>
> >>>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
> >>>>>>>>>>>> -                    "clock-frequency", NULL);
> >>>>>>>>>>>> -
> >>>>>>>>>>>> -    if (pfreq == NULL) {
> >>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
> >>>>>>>>>>>> +    if (rc) {
> >>>>>>>>>>>>           dev_warn(&pdev->dev,
> >>>>>>>>>>>>                "The watchdog clock frequency cannot be obtained\n");
> >>>>>>>>>>>>           no_timeout = true;
> >>>>>>>>>>>>       }
> >>>>>>>>>>>>
> >>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> >>>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
> >>>>>>>>>>>> -    if (tmptr == NULL) {
> >>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
> >>>>>>>>>>>> + &xdev->wdt_interval);
> >>>>>>>>>>>> +    if (rc) {
> >>>>>>>>>>>>           dev_warn(&pdev->dev,
> >>>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
> >>>>>>>>>>>>           no_timeout = true;
> >>>>>>>>>>>> -    } else {
> >>>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
> >>>>>>>>>>>>       }
> >>>>>>>>>>>>
> >>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> >>>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
> >>>>>>>>>>>> -    if (tmptr == NULL) {
> >>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
> >>>>>>>>>>>> + &enable_once);
> >>>>>>>>>>>> +    if (rc)
> >>>>>>>>>>>>           dev_warn(&pdev->dev,
> >>>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
> >>>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
> >>>>>>>>>>>> -    }
> >>>>>>>>>>> All the above properties are optional. Is a warning really
> >>>>>>>>>>> warranted in this case ? I usually associate a warning with
> >>>>>>>>>>> something that is wrong, which is not the case here.
> >>>>>>>>>>>
> >>>>>>>>>>> I would encourage you to drop those warnings, but that should be
> >>>>>>>>>>> a separate patch.
> >>>>>>>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
> >>>>>>>>> Hi
> >>>>>>>>>
> >>>>>>>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> The dt documentation states that the properties are optional.
> >>>>>>>>
> >>>>>>>> Optional properties:
> >>>>>>>> - clock-frequency       : Frequency of clock in Hz
> >>>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
> >>>>>>>>                           1 - Watchdog can be enabled just once
> >>>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
> >>>>>>>> <val> is integer from 8 to 31.
> >>>>>>>>
> >>>>>>>> This clearly conflicts with your statement. An optional property
> >>>>>>>> is just that, optional. If it warrants a warning, it must
> >>>>>>>> not be optional. If you claim that not providing the properties
> >>>>>>>> would be "wrong", why are they defined as optional ?
> >>>>>>> Hi Guenter
> >>>>>>>
> >>>>>>> I didn't know that these properties was classified as optional...
> >>>>>>> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
> >>>>>>> have these properties defined in theirs MPD files and theirs values can be obtained during the
> >>>>>>> hardware specification to device tree conversion.
> >>>>>>>> What is your definition of "wrong" and "must have" ?
> >>>>>>> what I mean for "must have" is: if these properties can be obtained
> >>>>>>> for all xilinx watchdog devices they must be present in the device tree because they allows
> >>>>>>> the system (linux/user) to know exactly how a watchdog device is configured.
> >>>>>>> Because these properties always can be obtained from hardware design there is no
> >>>>>>> reason to leave them out from the device tree. This is why I consider that a device tree without
> >>>>>>> these properties should be considered as "wrong" device tree.
> >>>>>>>> How do you expect anyone to know that omitting those
> >>>>>>>> "optional" properties is by some definition "wrong" ?
> >>>>>>> I'm agree with you.
> >>>>>>> Maybe these properties shouldn't be optional.
> >>>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
> >>>>>>> when a watchdog daemon ask for this property what is the obtained value ?
> >>>>>>> Independently of this value, why do not warn the user about this missing property
> >>>>>>> when it can always be in the device tree ?
> >>>>>>>
> >>>>>>
> >>>>>> Really, this line of argument doesn't make any sense to me.
> >>>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
> >>>>>> that the watchdog, when enabled, can not be stopped. It defaults
> >>>>>> to false, and thus is inherently optional. Making it mandatory
> >>>>>> doesn't really add any value.
> >>>>>>
> >>>>>
> >>>>> If the device has been configured with wdt-enable-once=true
> >>>>> and the device tree doesn't have this property then a watchdog daemon
> >>>>> would see it as "false" because it is the default making the system to misbehave...
> >>>>> A warning during driver loading could help user to identify the problem.
> >>>>>
> >>>>
> >>>> All this would give you is a false sense of safety. The property could
> >>>> just as well be there and be wrong (eg be configured as = <0> when it
> >>>> should be 1, or with a wrong frequency.
> >>> These issues "cannot" be detected but the missing properties yes.
> >>>> Following your logic, every driver
> >>>> would need to warn about everything, just to be sure.
> >>> Every driver should warn about anything that it considers weird and let the user to decide if it matters or not.
> >>> I think that an example of weird could be the lack of an expected property.
> >>>
> >>
> >> I don't think it makes sense to continue this discussion.
> >> We have fundamental differences in opinion which we won't
> >> resolve by repeating our arguments over and over.
> >>
> >> Wim, I'll let you decide how to handle this. My recommendation
> >> is to request the author to decide if the properties are optional
> >> or not before accepting this patch set. Either the properties
> >> are optional, and there should be no warnings, or they are
> >> mandatory and the driver should bail out if they are missing.
> >>
> > I'm totally agreed with you :)
> >
> 
> You have reached to completely different discussion.
> We should talk just about the code I have sent.
> I have checked what I have done and the intention was just to have better driver.
> I even didn't change any logic about DT and probe. DT binding just describes
> what it is written in the driver, nothing more nothing else.
> 
> Please keep this in your mind.
> 
> The driver with this binding is in the kernel for a while and if binding is wrong
> let's change it but it should be in separate patch which can fix binding
> warn/error messages.
> 
> Wim: If you agree, I have no problem to send this follow up patch
> which can be applied on the top of this series.
> We can make xlnx,wdt-enable-once and xlnx,wdt-interval as required properties
> and clock-frequence can go away and we can use CCF.

As said: a follow up patch would be welcome. So yes I agree.
And I also think that the discussion was interesting because it pointed out that a review of the bindings could also be usefull (what is required versus what os optional).

Kind regards,
Wim.

Re: [PATCH v3 0/11] Xilinx watchdog changes

[Wim Van Sebroeck]

Hi Michal,

> Hi,
> 
> This series contains changes for Xilinx watchdog.
> The whole code series have been reviewed by Guenter
> and device-tree binding was acked by Arnd.
> 
> I have also pushed this v3 to git repo
> for easier pulling here:
> git://git.monstr.eu/linux-2.6-microblaze.git watchdog
> 
> Thanks,
> Michal
> 
> Changes in v3:
> - Remove one if checking and use variable directly
> 
> Changes in v2:
> - Fix enable_once logic
> - Change patch subject
> - New patch in this series
> 
> Michal Simek (11):
>   watchdog: xilinx: Convert driver to the watchdog framework
>   watchdog: xilinx: Move control_status_reg to functions
>   watchdog: xilinx: Simplify probe and remove functions
>   watchdog: xilinx: Move no_timeout to probe function
>   watchdog: xilinx: Allocate private structure per device
>   watchdog: xilinx: Fix all printk messages
>   watchdog: xilinx: Use of_property_read_u32
>   watchdog: xilinx: Use correct comment indentation
>   watchdog: xilinx: Add missing binding
>   watchdog: xilinx: Enable this driver for Zynq
>   watchdog: xilinx: Remove no_timeout variable
> 
>  .../devicetree/bindings/watchdog/of-xilinx-wdt.txt |  23 ++
>  drivers/watchdog/Kconfig                           |  21 +-
>  drivers/watchdog/of_xilinx_wdt.c                   | 389 ++++++---------------
>  3 files changed, 148 insertions(+), 285 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
> 
> --
> 1.8.2.3
> 

This patch series has been added to linux-watchdog-next.

Kind regards,
Wim.

Re: [PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 9530 bytes --]

On 02/24/2014 08:25 PM, Wim Van Sebroeck wrote:
> Hi Michal,
> 
>> On 02/23/2014 08:00 PM, Alejandro Cabrera wrote:
>>> On 23/2/2014 6:43 AM, Guenter Roeck wrote:
>>>> On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
>>>>> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
>>>>>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>>>>>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>>>>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>>>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>>>>>>> Hi All,
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Michal,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Looks good.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Another comment/remark.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>>>>>>> -
>>>>>>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>>>>       }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>>>> -    } else {
>>>>>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>>>>>>       }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>>>>>>>>>> + &enable_once);
>>>>>>>>>>>>>> +    if (rc)
>>>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>>>>>>> -    }
>>>>>>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>>>>>>> a separate patch.
>>>>>>>>>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>>>>>>>>>> Hi
>>>>>>>>>>>
>>>>>>>>>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The dt documentation states that the properties are optional.
>>>>>>>>>>
>>>>>>>>>> Optional properties:
>>>>>>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>>>>>>                           1 - Watchdog can be enabled just once
>>>>>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>>>>>>>>>> <val> is integer from 8 to 31.
>>>>>>>>>>
>>>>>>>>>> This clearly conflicts with your statement. An optional property
>>>>>>>>>> is just that, optional. If it warrants a warning, it must
>>>>>>>>>> not be optional. If you claim that not providing the properties
>>>>>>>>>> would be "wrong", why are they defined as optional ?
>>>>>>>>> Hi Guenter
>>>>>>>>>
>>>>>>>>> I didn't know that these properties was classified as optional...
>>>>>>>>> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
>>>>>>>>> have these properties defined in theirs MPD files and theirs values can be obtained during the
>>>>>>>>> hardware specification to device tree conversion.
>>>>>>>>>> What is your definition of "wrong" and "must have" ?
>>>>>>>>> what I mean for "must have" is: if these properties can be obtained
>>>>>>>>> for all xilinx watchdog devices they must be present in the device tree because they allows
>>>>>>>>> the system (linux/user) to know exactly how a watchdog device is configured.
>>>>>>>>> Because these properties always can be obtained from hardware design there is no
>>>>>>>>> reason to leave them out from the device tree. This is why I consider that a device tree without
>>>>>>>>> these properties should be considered as "wrong" device tree.
>>>>>>>>>> How do you expect anyone to know that omitting those
>>>>>>>>>> "optional" properties is by some definition "wrong" ?
>>>>>>>>> I'm agree with you.
>>>>>>>>> Maybe these properties shouldn't be optional.
>>>>>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
>>>>>>>>> when a watchdog daemon ask for this property what is the obtained value ?
>>>>>>>>> Independently of this value, why do not warn the user about this missing property
>>>>>>>>> when it can always be in the device tree ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Really, this line of argument doesn't make any sense to me.
>>>>>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>>>>>>> that the watchdog, when enabled, can not be stopped. It defaults
>>>>>>>> to false, and thus is inherently optional. Making it mandatory
>>>>>>>> doesn't really add any value.
>>>>>>>>
>>>>>>>
>>>>>>> If the device has been configured with wdt-enable-once=true
>>>>>>> and the device tree doesn't have this property then a watchdog daemon
>>>>>>> would see it as "false" because it is the default making the system to misbehave...
>>>>>>> A warning during driver loading could help user to identify the problem.
>>>>>>>
>>>>>>
>>>>>> All this would give you is a false sense of safety. The property could
>>>>>> just as well be there and be wrong (eg be configured as = <0> when it
>>>>>> should be 1, or with a wrong frequency.
>>>>> These issues "cannot" be detected but the missing properties yes.
>>>>>> Following your logic, every driver
>>>>>> would need to warn about everything, just to be sure.
>>>>> Every driver should warn about anything that it considers weird and let the user to decide if it matters or not.
>>>>> I think that an example of weird could be the lack of an expected property.
>>>>>
>>>>
>>>> I don't think it makes sense to continue this discussion.
>>>> We have fundamental differences in opinion which we won't
>>>> resolve by repeating our arguments over and over.
>>>>
>>>> Wim, I'll let you decide how to handle this. My recommendation
>>>> is to request the author to decide if the properties are optional
>>>> or not before accepting this patch set. Either the properties
>>>> are optional, and there should be no warnings, or they are
>>>> mandatory and the driver should bail out if they are missing.
>>>>
>>> I'm totally agreed with you :)
>>>
>>
>> You have reached to completely different discussion.
>> We should talk just about the code I have sent.
>> I have checked what I have done and the intention was just to have better driver.
>> I even didn't change any logic about DT and probe. DT binding just describes
>> what it is written in the driver, nothing more nothing else.
>>
>> Please keep this in your mind.
>>
>> The driver with this binding is in the kernel for a while and if binding is wrong
>> let's change it but it should be in separate patch which can fix binding
>> warn/error messages.
>>
>> Wim: If you agree, I have no problem to send this follow up patch
>> which can be applied on the top of this series.
>> We can make xlnx,wdt-enable-once and xlnx,wdt-interval as required properties
>> and clock-frequence can go away and we can use CCF.
> 
> As said: a follow up patch would be welcome. So yes I agree.
> And I also think that the discussion was interesting because it pointed out that
 a review of the bindings could also be usefull (what is required versus what os optional).

Definitely it is not the first time where binding doc is useful.
Also from binding we can see which features are common for all
drivers in the same kind.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v3 07/11] watchdog: xilinx: Use of_property_read_u32

[Michal Simek]

On 02/24/2014 08:25 PM, Wim Van Sebroeck wrote:
> Hi Michal,
> 
>> On 02/23/2014 08:00 PM, Alejandro Cabrera wrote:
>>> On 23/2/2014 6:43 AM, Guenter Roeck wrote:
>>>> On 02/23/2014 08:25 AM, Alejandro Cabrera wrote:
>>>>> On 22/2/2014 7:44 PM, Guenter Roeck wrote:
>>>>>> On 02/22/2014 10:14 PM, Alejandro Cabrera wrote:
>>>>>>> On 22/2/2014 5:36 PM, Guenter Roeck wrote:
>>>>>>>> On 02/22/2014 07:52 PM, Alejandro Cabrera wrote:
>>>>>>>>> On 22/2/2014 3:18 PM, Guenter Roeck wrote:
>>>>>>>>>> On 02/22/2014 05:08 PM, Alejandro Cabrera wrote:
>>>>>>>>>>> On 22/2/2014 10:46 AM, Wim Van Sebroeck wrote:
>>>>>>>>>>>> Hi All,
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Michal,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Wed, Feb 12, 2014 at 02:41:21PM +0100, Michal Simek wrote:
>>>>>>>>>>>>>> Use of_property_read_u32 functions to clean probe function.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Michal Simek<michal.simek@xilinx.com>
>>>>>>>>>>>>>> Reviewed-by: Guenter Roeck<linux@roeck-us.net>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>>>> - Remove one if checking and use variable directly
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Looks good.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Another comment/remark.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> -    pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>>>> -                    "clock-frequency", NULL);
>>>>>>>>>>>>>> -
>>>>>>>>>>>>>> -    if (pfreq == NULL) {
>>>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency",&pfreq);
>>>>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>>>                "The watchdog clock frequency cannot be obtained\n");
>>>>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>>>>       }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>>>> -                    "xlnx,wdt-interval", NULL);
>>>>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>>>>>>>>>>>>> + &xdev->wdt_interval);
>>>>>>>>>>>>>> +    if (rc) {
>>>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>>>                "Parameter \"xlnx,wdt-interval\" not found\n");
>>>>>>>>>>>>>>           no_timeout = true;
>>>>>>>>>>>>>> -    } else {
>>>>>>>>>>>>>> -        xdev->wdt_interval = *tmptr;
>>>>>>>>>>>>>>       }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -    tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>>>>>>>>>>>>> -                    "xlnx,wdt-enable-once", NULL);
>>>>>>>>>>>>>> -    if (tmptr == NULL) {
>>>>>>>>>>>>>> +    rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>>>>>>>>>>>>> + &enable_once);
>>>>>>>>>>>>>> +    if (rc)
>>>>>>>>>>>>>>           dev_warn(&pdev->dev,
>>>>>>>>>>>>>>                "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>>>>>>>>>>>> -        watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>>>>>>>>>>>>> -    }
>>>>>>>>>>>>> All the above properties are optional. Is a warning really
>>>>>>>>>>>>> warranted in this case ? I usually associate a warning with
>>>>>>>>>>>>> something that is wrong, which is not the case here.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I would encourage you to drop those warnings, but that should be
>>>>>>>>>>>>> a separate patch.
>>>>>>>>>>>> I agree with Guenter: these are not really warnings. Seperate patch is thus welcome.
>>>>>>>>>>> Hi
>>>>>>>>>>>
>>>>>>>>>>> I support Michal intention, I think it is a warning because device tree blob must have the "xlnx,wdt-enable-once" property specified in order to allow the system to be sure of the real value of this property. In addition to, this warning can be helpful to detect a wrong device tree specification.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The dt documentation states that the properties are optional.
>>>>>>>>>>
>>>>>>>>>> Optional properties:
>>>>>>>>>> - clock-frequency       : Frequency of clock in Hz
>>>>>>>>>> - xlnx,wdt-enable-once  : 0 - Watchdog can be restarted
>>>>>>>>>>                           1 - Watchdog can be enabled just once
>>>>>>>>>> - xlnx,wdt-interval     : Watchdog timeout interval in 2^<val> clock cycles,
>>>>>>>>>> <val> is integer from 8 to 31.
>>>>>>>>>>
>>>>>>>>>> This clearly conflicts with your statement. An optional property
>>>>>>>>>> is just that, optional. If it warrants a warning, it must
>>>>>>>>>> not be optional. If you claim that not providing the properties
>>>>>>>>>> would be "wrong", why are they defined as optional ?
>>>>>>>>> Hi Guenter
>>>>>>>>>
>>>>>>>>> I didn't know that these properties was classified as optional...
>>>>>>>>> I think that they should not be, because all xilinx watchog devices (at least for microblaze processor)
>>>>>>>>> have these properties defined in theirs MPD files and theirs values can be obtained during the
>>>>>>>>> hardware specification to device tree conversion.
>>>>>>>>>> What is your definition of "wrong" and "must have" ?
>>>>>>>>> what I mean for "must have" is: if these properties can be obtained
>>>>>>>>> for all xilinx watchdog devices they must be present in the device tree because they allows
>>>>>>>>> the system (linux/user) to know exactly how a watchdog device is configured.
>>>>>>>>> Because these properties always can be obtained from hardware design there is no
>>>>>>>>> reason to leave them out from the device tree. This is why I consider that a device tree without
>>>>>>>>> these properties should be considered as "wrong" device tree.
>>>>>>>>>> How do you expect anyone to know that omitting those
>>>>>>>>>> "optional" properties is by some definition "wrong" ?
>>>>>>>>> I'm agree with you.
>>>>>>>>> Maybe these properties shouldn't be optional.
>>>>>>>>> For example suppose that "xlnx,wdt-enable-once" is missing in the device tree,
>>>>>>>>> when a watchdog daemon ask for this property what is the obtained value ?
>>>>>>>>> Independently of this value, why do not warn the user about this missing property
>>>>>>>>> when it can always be in the device tree ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Really, this line of argument doesn't make any sense to me.
>>>>>>>> "xlnx,wdt-enable-once", for example, is a boolean and means
>>>>>>>> that the watchdog, when enabled, can not be stopped. It defaults
>>>>>>>> to false, and thus is inherently optional. Making it mandatory
>>>>>>>> doesn't really add any value.
>>>>>>>>
>>>>>>>
>>>>>>> If the device has been configured with wdt-enable-once=true
>>>>>>> and the device tree doesn't have this property then a watchdog daemon
>>>>>>> would see it as "false" because it is the default making the system to misbehave...
>>>>>>> A warning during driver loading could help user to identify the problem.
>>>>>>>
>>>>>>
>>>>>> All this would give you is a false sense of safety. The property could
>>>>>> just as well be there and be wrong (eg be configured as = <0> when it
>>>>>> should be 1, or with a wrong frequency.
>>>>> These issues "cannot" be detected but the missing properties yes.
>>>>>> Following your logic, every driver
>>>>>> would need to warn about everything, just to be sure.
>>>>> Every driver should warn about anything that it considers weird and let the user to decide if it matters or not.
>>>>> I think that an example of weird could be the lack of an expected property.
>>>>>
>>>>
>>>> I don't think it makes sense to continue this discussion.
>>>> We have fundamental differences in opinion which we won't
>>>> resolve by repeating our arguments over and over.
>>>>
>>>> Wim, I'll let you decide how to handle this. My recommendation
>>>> is to request the author to decide if the properties are optional
>>>> or not before accepting this patch set. Either the properties
>>>> are optional, and there should be no warnings, or they are
>>>> mandatory and the driver should bail out if they are missing.
>>>>
>>> I'm totally agreed with you :)
>>>
>>
>> You have reached to completely different discussion.
>> We should talk just about the code I have sent.
>> I have checked what I have done and the intention was just to have better driver.
>> I even didn't change any logic about DT and probe. DT binding just describes
>> what it is written in the driver, nothing more nothing else.
>>
>> Please keep this in your mind.
>>
>> The driver with this binding is in the kernel for a while and if binding is wrong
>> let's change it but it should be in separate patch which can fix binding
>> warn/error messages.
>>
>> Wim: If you agree, I have no problem to send this follow up patch
>> which can be applied on the top of this series.
>> We can make xlnx,wdt-enable-once and xlnx,wdt-interval as required properties
>> and clock-frequence can go away and we can use CCF.
> 
> As said: a follow up patch would be welcome. So yes I agree.
> And I also think that the discussion was interesting because it pointed out that
 a review of the bindings could also be usefull (what is required versus what os optional).

Definitely it is not the first time where binding doc is useful.
Also from binding we can see which features are common for all
drivers in the same kind.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140225/63097b45/attachment-0001.sig>