Hi Rajendra,

On 9/27/2011 7:40 AM, Nayak, Rajendra wrote:

On Monday 26 September 2011 10:20 PM, Benoit Cousson wrote:

quoted

Adapt the GPIO driver to retrieve information from a DT file.
Note that since the driver is currently under cleanup, some hacks
will have to be removed after.

Add documentation for GPIO properties specific to OMAP.

Remove an un-needed whitespace.

Signed-off-by: Benoit Cousson<redacted>
Cc: Grant Likely<redacted>
Cc: Charulatha V<redacted>
Cc: Tarun Kanti DebBarma<redacted>
---
   .../devicetree/bindings/gpio/gpio-omap.txt         |   33 ++++++
   drivers/gpio/gpio-omap.c                           |  108 ++++++++++++++++++--
   2 files changed, 132 insertions(+), 9 deletions(-)
   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-omap.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
new file mode 100644
index 0000000..bdd63de
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt

@@ -0,0 +1,33 @@
+OMAP GPIO controller
+
+Required properties:
+- compatible:
+  - "ti,omap2-gpio" for OMAP2 and OMAP3 controllers

Would it be more readable to have
"ti,omap2-gpio" for OMAP2 controllers and
"ti,omap3-gpio" for OMAP3 controllers?

quoted

+  - "ti,omap4-gpio" for OMAP4 controller
+- #gpio-cells : Should be two.
+  - first cell is the pin number
+  - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller.
+
+OMAP specific properties:
+- ti,hwmods: Name of the hwmod associated to the GPIO
+- id: 32 bits to identify the id (1 based index)
+- bank-width: number of pin supported by the controller (16 or 32)
+- debounce: set if the controller support the debouce funtionnality
+- bank-count: number of controller support by the SoC. This is a temporary
+  hack until the bank_count is removed from the driver.

Is there a general rule to be followed as to when to use
"ti,<prop-name>" and when to use just"<prop-name>".
Since all these are OMAP specific properties, shouldn't all
of them be "ti,<prop-name>"?

To be honest, I was wondering as well about this rule.
I think that a property that is not purely OMAP specific and that 
represents some standard HW information does not have to be prefixed by 
"ti,XXX".
So hwmods must be "ti,hwmods", but bank-witdh and bank-count seems to me 
quite generic.

quoted

+Example:
+
+gpio4: gpio4 {
+    compatible = "ti,omap4-gpio", "ti,omap-gpio";
+    ti,hwmods = "gpio4";
+    id =<4>;
+    bank-width =<32>;
+    debounce;
+    no_idle_on_suspend;
+    #gpio-cells =<2>;
+    gpio-controller;
+};
+

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 0599854..f878fa4 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c

@@ -21,6 +21,8 @@
   #include<linux/io.h>
   #include<linux/slab.h>
   #include<linux/pm_runtime.h>
+#include<linux/of.h>
+#include<linux/of_device.h>

   #include<mach/hardware.h>
   #include<asm/irq.h>

@@ -521,7 +523,7 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable)
   	unsigned long flags;

   	if (bank->non_wakeup_gpios&   gpio_bit) {
-		dev_err(bank->dev,
+		dev_err(bank->dev,

Stray change?

Not anymore, it is part of the changelog :-)

quoted

   			"Unable to modify wakeup on non-wakeup GPIO%d\n", gpio);
   		return -EINVAL;
   	}

@@ -1150,6 +1152,8 @@ static void __devinit omap_gpio_chip_init(struct gpio_bank *bank)
   	irq_set_handler_data(bank->irq, bank);
   }

+static const struct of_device_id omap_gpio_match[];
+
   static int __devinit omap_gpio_probe(struct platform_device *pdev)
   {
   	static int gpio_init_done;

@@ -1157,11 +1161,31 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
   	struct resource *res;
   	int id;
   	struct gpio_bank *bank;
+	struct device_node *node = pdev->dev.of_node;
+	const struct of_device_id *match;
+
+	match = of_match_device(omap_gpio_match,&pdev->dev);
+	if (match) {
+		pdata = match->data;
+		/* XXX: big hack until the bank_count is removed */
+		of_property_read_u32(node, "bank-count",&gpio_bank_count);
+		if (of_property_read_u32(node, "id",&id))

id should be u32.

Oops, good point.

quoted

+			return -EINVAL;
+		/*
+		 * In an ideal world, the id should not be needed, but since
+		 * the OMAP TRM consider the multiple GPIO controllers as
+		 * multiple banks, the GPIO number is based on the whole set
+		 * of banks. Hence the need to provide an id in order to
+		 * respect the order and the correct GPIO number.
+		 */
+		id -= 1;
+	} else {
+		if (!pdev->dev.platform_data)
+			return -EINVAL;

-	if (!pdev->dev.platform_data)
-		return -EINVAL;
-
-	pdata = pdev->dev.platform_data;
+		pdata = pdev->dev.platform_data;
+		id = pdev->id;
+	}

   	if (!gpio_init_done) {
   		int ret;

@@ -1171,7 +1195,6 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
   			return ret;
   	}

-	id = pdev->id;
   	bank =&gpio_bank[id];

   	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

@@ -1181,12 +1204,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
   	}

   	bank->irq = res->start;
-	bank->virtual_irq_start = pdata->virtual_irq_start;
   	bank->method = pdata->bank_type;
   	bank->dev =&pdev->dev;
-	bank->dbck_flag = pdata->dbck_flag;
   	bank->stride = pdata->bank_stride;
-	bank->width = pdata->bank_width;
+	if (match) {
+		of_property_read_u32(node, "bank-width",&bank->width);

Bank width should be u32.

quoted

+		if (of_get_property(node, "debounce", NULL))

of_find_property() should suffice.

Yes, indeed.

Thanks,
Benoit