[09/11] gpio: davinci: DT changes for driver

Commit Message

avinash philip May 22, 2013, 7:10 a.m. UTC

From: KV Sujith <[e-mail]>

- Add of_device_id for Davinci GPIO driver.
- Add function to populate data from DT.
- Modify the probe to read from DT if DT match is found.
- Add DT binding documentation for Davinci GPIO properties in a new file
  gpio-davinci.txt located at Documentation/devicetree/bindings/gpio/.

Cc: Grant Likely <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Rob Landley <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: KV Sujith <[e-mail]>
Signed-off-by: Philip Avinash <[e-mail]>
---
 .../devicetree/bindings/gpio/gpio-davinci.txt      |   26 +++++++++
 drivers/gpio/gpio-davinci.c                        |   57 ++++++++++++++++++--
 2 files changed, 80 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt

Comments

Linus Walleij May 30, 2013, 6:25 p.m. UTC | #1

On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[e-mail]> wrote:

(...)
> +- interrupts: The Starting IRQ number for GPIO
> +- intc_irq_num: The number of IRQs supported by the Interrupt Controller
(...)

No this is not how you pass a number of IRQs in the device tree.

"interrupts" is an array. Pass every interrupt here for a full
resolution of the IRQs.

Further this looks fishy:

+ interrupts = <42>;

Usually you pass flags with the IRQs, I would rather have expected
an array like this:

interrupts = < 90 0x4 96 0x4 14 0x4 15 0x4 79 0x4>;

0x4 is IRQ_TYPE_LEVEL_HIGH, you can use the dts
#include <dt-bindings/interrupt-controller/irq.h> and
define that symbolically.

Doesn't the DaVinci IRQ controller support *any* IRQ flags?

Since the driver code is not reading out the interrupts but
(I guess?) falling back to platform data IRQ assignment,
this seems wrong.

Yours,
Linus Walleij

avinash philip June 10, 2013, 11:45 a.m. UTC | #2

On Thu, May 30, 2013 at 23:55:22, Linus Walleij wrote:
> On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[e-mail]> wrote:
> 
> (...)
> > +- interrupts: The Starting IRQ number for GPIO
> > +- intc_irq_num: The number of IRQs supported by the Interrupt Controller
> (...)
> 
> No this is not how you pass a number of IRQs in the device tree.
> 
> "interrupts" is an array. Pass every interrupt here for a full
> resolution of the IRQs.

Correct. I will change.

> 
> Further this looks fishy:
> 
> + interrupts = <42>;
> 
> Usually you pass flags with the IRQs, I would rather have expected
> an array like this:
> 
> interrupts = < 90 0x4 96 0x4 14 0x4 15 0x4 79 0x4>;
> 
> 0x4 is IRQ_TYPE_LEVEL_HIGH, you can use the dts
> #include <dt-bindings/interrupt-controller/irq.h> and
> define that symbolically.
> 
> Doesn't the DaVinci IRQ controller support *any* IRQ flags?

I wasn't sure about it.
But from davinci GPIO driver perspective, GPIO pins are
configured as edge sensitive. So IRQ_TYPE_EDGE_BOTH can be used.

So I will correct Documentation and update DT nodes in next version.

> 
> Since the driver code is not reading out the interrupts but
> (I guess?) falling back to platform data IRQ assignment,
> this seems wrong.

Driver code reads "Starting IRQ number for GPIO" from platform resource
See [PATCH 03/11] gpio: davinci: Modify to platform driver.
Driver requires only starting offset of gpio irq number. GPIO interrupt
Number expected in sequential order for davinci GPIO.

Thanks
Avinash

 
> Yours,
> Linus Walleij
>

Patch

diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
new file mode 100644
index 0000000..0d599d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
@@ -0,0 +1,26 @@ 
+Davinci GPIO controller bindings
+
+Required Properties:
+- compatible:"ti,da830-gpio"
+
+- reg: Physical base address of the controller and length of memory mapped
+	region.
+
+- interrupts: The Starting IRQ number for GPIO
+
+- ngpio: The number of GPIO pins supported
+
+- intc_irq_num: The number of IRQs supported by the Interrupt Controller
+
+- gpio_unbanked: The number of GPIOs that have an individual interrupt
+		line to processor.
+
+Example:
+gpio: gpio@1e26000 {
+	compatible = "ti,da830-gpio";
+	reg = <0x226000 0x1000>;
+	interrupts = <42>;
+	ngpio = <144>;
+	intc_irq_num = <101>;
+	gpio_unbanked = <0>;
+};
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 08830aa..dbe3b83 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -19,6 +19,8 @@ 
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/gpio-davinci.h>
 #include <mach/gpio-davinci.h>
@@ -133,6 +135,50 @@  static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset,
 	__raw_writel((1 << offset), value ? &regs->set_data : &regs->clr_data);
 }
 
+static struct davinci_gpio_platform_data *davinci_gpio_set_pdata_of(
+						struct platform_device *pdev)
+{
+	struct device_node *dn = pdev->dev.of_node;
+	struct davinci_gpio_platform_data *pdata;
+	u32 val, ret;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (pdata) {
+		ret = of_property_read_u32(dn, "ngpio", &val);
+		if (ret)
+			goto of_err;
+
+		pdata->ngpio = val;
+
+		ret = of_property_read_u32(dn, "gpio_unbanked", &val);
+		if (ret)
+			goto of_err;
+
+		pdata->gpio_unbanked = val;
+
+		ret = of_property_read_u32(dn, "intc_irq_num", &val);
+		if (ret)
+			goto of_err;
+
+		pdata->intc_irq_num = val;
+	}
+
+	return pdata;
+
+of_err:
+	dev_err(&pdev->dev, "Populating pdata from DT failed: err %d\n", ret);
+	return NULL;
+}
+
+static const struct of_device_id davinci_gpio_ids[] = {
+	{
+		.compatible = "ti,da830-gpio",
+	},
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, davinci_gpio_ids);
+
 static int davinci_gpio_probe(struct platform_device *pdev)
 {
 	int i, base;
@@ -142,13 +188,17 @@  static int davinci_gpio_probe(struct platform_device *pdev)
 	struct davinci_gpio_regs *regs;
 	struct device *dev = &pdev->dev;
 	struct resource *res;
+	const struct of_device_id *match =
+		of_match_device(of_match_ptr(davinci_gpio_ids), &pdev->dev);
 
-	pdata = dev->platform_data;
+	pdata = match ? davinci_gpio_set_pdata_of(pdev) : dev->platform_data;
 	if (!pdata) {
 		dev_err(dev, "GPIO: No Platform Data Supplied\n");
 		return -EINVAL;
 	}
 
+	dev->platform_data = pdata;
+
 	/*
 	 * The gpio banks conceptually expose a segmented bitmap,
 	 * and "ngpio" is one more than the largest zero-based
@@ -490,8 +540,9 @@  done:
 static struct platform_driver davinci_gpio_driver = {
 	.probe		= davinci_gpio_probe,
 	.driver		= {
-		.name	= "davinci_gpio",
-		.owner	= THIS_MODULE,
+		.name		= "davinci_gpio",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(davinci_gpio_ids),
 	},
 };