[PATCH 0/5] gpio/omap: Cleanup and adaptation to Device Tree

[PATCH 0/5] gpio/omap: Cleanup and adaptation to Device Tree

[Benoit Cousson]

Hi Grant,

Here are a couple of GPIO cleanup + the DT adaptation patches.

This series is based on lo/master (a37cdd9) + for_3.4/dt_base branch to get
the needed cleanup and fixes for OMAP.

The interrupt controller support is using irq_domain_add_legacy for
the moment and will be updated next to use irqchip irq_domain.

It requires the irq_domain generalization and refinement series
available at:
  git://git.secretlab.ca/git/linux-2.6 irqdomain/next
  
But I guess you already know where to find it :-)

It requires as well Tarun's GPIO cleanup series available at:
  git://gitorious.org/~tarunkanti/omap-sw-develoment/tarunkantis-linux-omap-dev for_3.4/gpio_cleanup_fixes_v9

This series is available here for reference:
  git://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git for_3.4/dt_gpio

Regards,
Benoit


Benoit Cousson (5):
  gpio/omap: Remove bank->id information and misc cleanup
  gpio/omap: Use devm_ API and add request_mem_region
  gpio/omap: Add DT support to GPIO driver
  arm/dts: OMAP4: Add gpio nodes
  arm/dts: OMAP3: Add gpio nodes

 .../devicetree/bindings/gpio/gpio-omap.txt         |   30 ++++
 arch/arm/boot/dts/omap3.dtsi                       |   54 ++++++
 arch/arm/boot/dts/omap4.dtsi                       |   54 ++++++
 arch/arm/mach-omap2/gpio.c                         |    8 +-
 drivers/gpio/gpio-omap.c                           |  175 +++++++++++++++-----
 5 files changed, 278 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-omap.txt

[PATCH 1/5] gpio/omap: Remove bank->id information and misc cleanup

[Benoit Cousson]

The driver does not need anymore any id to identify the GPIO instance.
Remove every occurence of the bank->id inside the driver.

Remove two trailing spaces.
Add a dev variable for better readability in probe.
Remove unused variable bank->pbase.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index f49bd6f..a0c3e03 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -50,7 +50,6 @@ struct gpio_regs {
 
 struct gpio_bank {
 	struct list_head node;
-	unsigned long pbase;
 	void __iomem *base;
 	u16 irq;
 	u16 virtual_irq_start;
@@ -77,7 +76,6 @@ struct gpio_bank {
 	int stride;
 	u32 width;
 	int context_loss_count;
-	u16 id;
 	int power_mode;
 	bool workaround_enabled;
 
@@ -155,7 +153,7 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
 {
 	int l = __raw_readl(base + reg);
 
-	if (set) 
+	if (set)
 		l |= mask;
 	else
 		l &= ~mask;
@@ -495,7 +493,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,
 			"Unable to modify wakeup on non-wakeup GPIO%d\n", gpio);
 		return -EINVAL;
 	}
@@ -1048,37 +1046,36 @@ static void __devinit omap_gpio_chip_init(struct gpio_bank *bank)
 
 static int __devinit omap_gpio_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct omap_gpio_platform_data *pdata;
 	struct resource *res;
 	struct gpio_bank *bank;
 	int ret = 0;
 
-	if (!pdev->dev.platform_data) {
+	if (!dev->platform_data) {
 		ret = -EINVAL;
 		goto err_exit;
 	}
 
 	bank = kzalloc(sizeof(struct gpio_bank), GFP_KERNEL);
 	if (!bank) {
-		dev_err(&pdev->dev, "Memory alloc failed for gpio_bank\n");
+		dev_err(dev, "Memory alloc failed\n");
 		ret = -ENOMEM;
 		goto err_exit;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (unlikely(!res)) {
-		dev_err(&pdev->dev, "GPIO Bank %i Invalid IRQ resource\n",
-				pdev->id);
+		dev_err(dev, "Invalid IRQ resource\n");
 		ret = -ENODEV;
 		goto err_free;
 	}
 
 	bank->irq = res->start;
-	bank->id = pdev->id;
 
 	pdata = pdev->dev.platform_data;
 	bank->virtual_irq_start = pdata->virtual_irq_start;
-	bank->dev = &pdev->dev;
+	bank->dev = dev;
 	bank->dbck_flag = pdata->dbck_flag;
 	bank->stride = pdata->bank_stride;
 	bank->width = pdata->bank_width;
@@ -1098,16 +1095,14 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	/* Static mapping, never released */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (unlikely(!res)) {
-		dev_err(&pdev->dev, "GPIO Bank %i Invalid mem resource\n",
-				pdev->id);
+		dev_err(dev, "Invalid mem resource\n");
 		ret = -ENODEV;
 		goto err_free;
 	}
 
 	bank->base = ioremap(res->start, resource_size(res));
 	if (!bank->base) {
-		dev_err(&pdev->dev, "Could not ioremap gpio bank%i\n",
-				pdev->id);
+		dev_err(dev, "Could not ioremap\n");
 		ret = -ENOMEM;
 		goto err_free;
 	}
-- 
1.7.0.4

[PATCH 2/5] gpio/omap: Use devm_ API and add request_mem_region

[Benoit Cousson]

Replace the regular kzalloc and ioremap with the devm_ equivalent
to simplify error handling.

Add the missing devm_request_mem_region to reserve the region used
by the driver.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
 drivers/gpio/gpio-omap.c |   35 +++++++++++++++--------------------
 1 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index a0c3e03..c3a9dc8 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -19,7 +19,7 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/io.h>
-#include <linux/slab.h>
+#include <linux/device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm.h>
 
@@ -1052,23 +1052,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	struct gpio_bank *bank;
 	int ret = 0;
 
-	if (!dev->platform_data) {
-		ret = -EINVAL;
-		goto err_exit;
-	}
+	if (!dev->platform_data)
+		return -EINVAL;
 
-	bank = kzalloc(sizeof(struct gpio_bank), GFP_KERNEL);
+	bank = devm_kzalloc(&pdev->dev, sizeof(struct gpio_bank), GFP_KERNEL);
 	if (!bank) {
 		dev_err(dev, "Memory alloc failed\n");
-		ret = -ENOMEM;
-		goto err_exit;
+		return -ENOMEM;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (unlikely(!res)) {
 		dev_err(dev, "Invalid IRQ resource\n");
-		ret = -ENODEV;
-		goto err_free;
+		return -ENODEV;
 	}
 
 	bank->irq = res->start;
@@ -1096,15 +1092,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (unlikely(!res)) {
 		dev_err(dev, "Invalid mem resource\n");
-		ret = -ENODEV;
-		goto err_free;
+		return -ENODEV;
+	}
+
+	if (!devm_request_mem_region(dev, res->start, resource_size(res),
+				     pdev->name)) {
+		dev_err(dev, "Region already claimed\n");
+		return -EBUSY;
 	}
 
-	bank->base = ioremap(res->start, resource_size(res));
+	bank->base = devm_ioremap(dev, res->start, resource_size(res));
 	if (!bank->base) {
 		dev_err(dev, "Could not ioremap\n");
-		ret = -ENOMEM;
-		goto err_free;
+		return -ENOMEM;
 	}
 
 	platform_set_drvdata(pdev, bank);
@@ -1125,11 +1125,6 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	list_add_tail(&bank->node, &omap_gpio_list);
 
 	return ret;
-
-err_free:
-	kfree(bank);
-err_exit:
-	return ret;
 }
 
 #ifdef CONFIG_ARCH_OMAP2PLUS
-- 
1.7.0.4

[PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Benoit Cousson]

Adapt the GPIO driver to retrieve information from a DT file.

Allocate the irq_base dynamically and rename bank->virtual_irq_start
to bank->irq_base.
Change irq_base type to int instead of u16 to match irq_alloc_descs
output.

Add documentation for GPIO properties specific to OMAP.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
 .../devicetree/bindings/gpio/gpio-omap.txt         |   30 +++++
 drivers/gpio/gpio-omap.c                           |  121 ++++++++++++++++++--
 2 files changed, 142 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..c1b3100
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
@@ -0,0 +1,30 @@
+OMAP GPIO controller bindings
+
+Required properties:
+- compatible:
+  - "ti,omap2-gpio" for OMAP2 controllers
+  - "ti,omap3-gpio" for OMAP3 controllers
+  - "ti,omap4-gpio" for OMAP4 controllers
+- #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.
+- #interrupt-cells : Should be one
+- interrupt-controller: Mark the device node as an interrupt controller
+
+OMAP specific properties:
+- ti,hwmods: Name of the hwmod associated to the GPIO:
+  "gpio<X>", <X> being the 1-based instance number from the HW spec
+
+
+Example:
+
+gpio4: gpio4 {
+    compatible = "ti,omap4-gpio";
+    ti,hwmods = "gpio4";
+    #gpio-cells = <2>;
+    gpio-controller;
+    #interrupt-cells = <1>;
+    interrupt-controller;
+};
+
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index c3a9dc8..bc2bd69 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -22,6 +22,9 @@
 #include <linux/device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/irqdomain.h>
 
 #include <mach/hardware.h>
 #include <asm/irq.h>
@@ -52,7 +55,8 @@ struct gpio_bank {
 	struct list_head node;
 	void __iomem *base;
 	u16 irq;
-	u16 virtual_irq_start;
+	int irq_base;
+	struct irq_domain *domain;
 	u32 suspend_wakeup;
 	u32 saved_wakeup;
 	u32 non_wakeup_gpios;
@@ -669,7 +673,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 		if (!isr)
 			break;
 
-		gpio_irq = bank->virtual_irq_start;
+		gpio_irq = bank->irq_base;
 		for (; isr != 0; isr >>= 1, gpio_irq++) {
 			gpio_index = GPIO_INDEX(bank, irq_to_gpio(gpio_irq));
 
@@ -915,7 +919,7 @@ static int gpio_2irq(struct gpio_chip *chip, unsigned offset)
 	struct gpio_bank *bank;
 
 	bank = container_of(chip, struct gpio_bank, chip);
-	return bank->virtual_irq_start + offset;
+	return bank->irq_base + offset;
 }
 
 /*---------------------------------------------------------------------*/
@@ -1028,8 +1032,7 @@ static void __devinit omap_gpio_chip_init(struct gpio_bank *bank)
 
 	gpiochip_add(&bank->chip);
 
-	for (j = bank->virtual_irq_start;
-		     j < bank->virtual_irq_start + bank->width; j++) {
+	for (j = bank->irq_base; j < bank->irq_base + bank->width; j++) {
 		irq_set_lockdep_class(j, &gpio_lock_class);
 		irq_set_chip_data(j, bank);
 		if (bank->is_mpuio) {
@@ -1044,15 +1047,22 @@ 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)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	const struct of_device_id *match;
 	struct omap_gpio_platform_data *pdata;
 	struct resource *res;
 	struct gpio_bank *bank;
 	int ret = 0;
 
-	if (!dev->platform_data)
+	match = of_match_device(of_match_ptr(omap_gpio_match), dev);
+
+	pdata = match ? match->data : dev->platform_data;
+	if (!pdata)
 		return -EINVAL;
 
 	bank = devm_kzalloc(&pdev->dev, sizeof(struct gpio_bank), GFP_KERNEL);
@@ -1068,9 +1078,6 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	}
 
 	bank->irq = res->start;
-
-	pdata = pdev->dev.platform_data;
-	bank->virtual_irq_start = pdata->virtual_irq_start;
 	bank->dev = dev;
 	bank->dbck_flag = pdata->dbck_flag;
 	bank->stride = pdata->bank_stride;
@@ -1080,6 +1087,18 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	bank->loses_context = pdata->loses_context;
 	bank->get_context_loss_count = pdata->get_context_loss_count;
 	bank->regs = pdata->regs;
+#ifdef CONFIG_OF_GPIO
+	bank->chip.of_node = of_node_get(node);
+#endif
+
+	bank->irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
+	if (bank->irq_base < 0) {
+		dev_err(dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+
+	bank->domain = irq_domain_add_legacy(node, bank->width, bank->irq_base,
+					     0, &irq_domain_simple_ops, NULL);
 
 	if (bank->regs->set_dataout && bank->regs->clr_dataout)
 		bank->set_dataout = _set_gpio_dataout_reg;
@@ -1387,11 +1406,95 @@ static const struct dev_pm_ops gpio_pm_ops = {
 									NULL)
 };
 
+#if defined(CONFIG_OF)
+static struct omap_gpio_reg_offs omap2_gpio_regs = {
+	.revision =		OMAP24XX_GPIO_REVISION,
+	.direction =		OMAP24XX_GPIO_OE,
+	.datain =		OMAP24XX_GPIO_DATAIN,
+	.dataout =		OMAP24XX_GPIO_DATAOUT,
+	.set_dataout =		OMAP24XX_GPIO_SETDATAOUT,
+	.clr_dataout =		OMAP24XX_GPIO_CLEARDATAOUT,
+	.irqstatus =		OMAP24XX_GPIO_IRQSTATUS1,
+	.irqstatus2 =		OMAP24XX_GPIO_IRQSTATUS2,
+	.irqenable =		OMAP24XX_GPIO_IRQENABLE1,
+	.irqenable2 =		OMAP24XX_GPIO_IRQENABLE2,
+	.set_irqenable =	OMAP24XX_GPIO_SETIRQENABLE1,
+	.clr_irqenable =	OMAP24XX_GPIO_CLEARIRQENABLE1,
+	.debounce =		OMAP24XX_GPIO_DEBOUNCE_VAL,
+	.debounce_en =		OMAP24XX_GPIO_DEBOUNCE_EN,
+	.ctrl =			OMAP24XX_GPIO_CTRL,
+	.wkup_en =		OMAP24XX_GPIO_WAKE_EN,
+	.leveldetect0 =		OMAP24XX_GPIO_LEVELDETECT0,
+	.leveldetect1 =		OMAP24XX_GPIO_LEVELDETECT1,
+	.risingdetect =		OMAP24XX_GPIO_RISINGDETECT,
+	.fallingdetect =	OMAP24XX_GPIO_FALLINGDETECT,
+};
+
+static struct omap_gpio_reg_offs omap4_gpio_regs = {
+	.revision =		OMAP4_GPIO_REVISION,
+	.direction =		OMAP4_GPIO_OE,
+	.datain =		OMAP4_GPIO_DATAIN,
+	.dataout =		OMAP4_GPIO_DATAOUT,
+	.set_dataout =		OMAP4_GPIO_SETDATAOUT,
+	.clr_dataout =		OMAP4_GPIO_CLEARDATAOUT,
+	.irqstatus =		OMAP4_GPIO_IRQSTATUS0,
+	.irqstatus2 =		OMAP4_GPIO_IRQSTATUS1,
+	.irqenable =		OMAP4_GPIO_IRQSTATUSSET0,
+	.irqenable2 =		OMAP4_GPIO_IRQSTATUSSET1,
+	.set_irqenable =	OMAP4_GPIO_IRQSTATUSSET0,
+	.clr_irqenable =	OMAP4_GPIO_IRQSTATUSCLR0,
+	.debounce =		OMAP4_GPIO_DEBOUNCINGTIME,
+	.debounce_en =		OMAP4_GPIO_DEBOUNCENABLE,
+	.ctrl =			OMAP4_GPIO_CTRL,
+	.wkup_en =		OMAP4_GPIO_IRQWAKEN0,
+	.leveldetect0 =		OMAP4_GPIO_LEVELDETECT0,
+	.leveldetect1 =		OMAP4_GPIO_LEVELDETECT1,
+	.risingdetect =		OMAP4_GPIO_RISINGDETECT,
+	.fallingdetect =	OMAP4_GPIO_FALLINGDETECT,
+};
+
+static struct omap_gpio_platform_data omap2_pdata = {
+	.regs = &omap2_gpio_regs,
+	.bank_width = 32,
+	.dbck_flag = false,
+};
+
+static struct omap_gpio_platform_data omap3_pdata = {
+	.regs = &omap2_gpio_regs,
+	.bank_width = 32,
+	.dbck_flag = true,
+};
+
+static struct omap_gpio_platform_data omap4_pdata = {
+	.regs = &omap4_gpio_regs,
+	.bank_width = 32,
+	.dbck_flag = true,
+};
+
+static const struct of_device_id omap_gpio_match[] = {
+	{
+		.compatible = "ti,omap4-gpio",
+		.data = &omap4_pdata,
+	},
+	{
+		.compatible = "ti,omap3-gpio",
+		.data = &omap3_pdata,
+	},
+	{
+		.compatible = "ti,omap2-gpio",
+		.data = &omap2_pdata,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, omap_gpio_match);
+#endif
+
 static struct platform_driver omap_gpio_driver = {
 	.probe		= omap_gpio_probe,
 	.driver		= {
 		.name	= "omap_gpio",
 		.pm	= &gpio_pm_ops,
+		.of_match_table = of_match_ptr(omap_gpio_match),
 	},
 };
 
-- 
1.7.0.4

[PATCH 4/5] arm/dts: OMAP4: Add gpio nodes

[Benoit Cousson]

Add the 6 GPIOs controller nodes present in OMAP4.

Remove gpio static device initialisation if DT is populated.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 arch/arm/boot/dts/omap4.dtsi |   54 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/gpio.c   |    8 ++++-
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 29f4589..4b81f03 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -104,6 +104,60 @@
 			      <0x48240100 0x0100>;
 		};
 
+		gpio1: gpio@1 {
+			compatible = "ti,omap4-gpio";
+			ti,hwmods = "gpio1";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio2: gpio@2 {
+			compatible = "ti,omap4-gpio";
+			ti,hwmods = "gpio2";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio3: gpio@3 {
+			compatible = "ti,omap4-gpio";
+			ti,hwmods = "gpio3";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio4: gpio@4 {
+			compatible = "ti,omap4-gpio";
+			ti,hwmods = "gpio4";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio5: gpio@5 {
+			compatible = "ti,omap4-gpio";
+			ti,hwmods = "gpio5";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio6: gpio@6 {
+			compatible = "ti,omap4-gpio";
+			ti,hwmods = "gpio6";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		uart1: serial@0x4806a000 {
 			compatible = "ti,omap4-uart";
 			ti,hwmods = "uart1";
diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c
index 1e0b750..de22abd 100644
--- a/arch/arm/mach-omap2/gpio.c
+++ b/arch/arm/mach-omap2/gpio.c
@@ -20,6 +20,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 
 #include <plat/omap_hwmod.h>
 #include <plat/omap_device.h>
@@ -146,7 +147,10 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused)
  */
 static int __init omap2_gpio_init(void)
 {
-	return omap_hwmod_for_each_by_class("gpio", omap2_gpio_dev_init,
-						NULL);
+	/* If dtb is there, the devices will be created dynamically */
+	if (of_have_populated_dt())
+		return -ENODEV;
+
+	return omap_hwmod_for_each_by_class("gpio", omap2_gpio_dev_init, NULL);
 }
 postcore_initcall(omap2_gpio_init);
-- 
1.7.0.4

[PATCH 5/5] arm/dts: OMAP3: Add gpio nodes

[Benoit Cousson]

Add the 6 GPIOs controller nodes present in OMAP3.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 arch/arm/boot/dts/omap3.dtsi |   54 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index ca37ba5..8eb181e 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -69,6 +69,60 @@
 			reg = <0x48200000 0x1000>;
 		};
 
+		gpio1: gpio@1 {
+			compatible = "ti,omap3-gpio";
+			ti,hwmods = "gpio1";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio2: gpio@2 {
+			compatible = "ti,omap3-gpio";
+			ti,hwmods = "gpio2";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio3: gpio@3 {
+			compatible = "ti,omap3-gpio";
+			ti,hwmods = "gpio3";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio4: gpio@4 {
+			compatible = "ti,omap3-gpio";
+			ti,hwmods = "gpio4";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio5: gpio@5 {
+			compatible = "ti,omap3-gpio";
+			ti,hwmods = "gpio5";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		gpio6: gpio@6 {
+			compatible = "ti,omap3-gpio";
+			ti,hwmods = "gpio6";
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
 		uart1: serial@0x4806a000 {
 			compatible = "ti,omap3-uart";
 			ti,hwmods = "uart1";
-- 
1.7.0.4

Re: [PATCH 2/5] gpio/omap: Use devm_ API and add request_mem_region

[DebBarma, Tarun Kanti]

Hi Benoit,

On Wed, Feb 15, 2012 at 9:34 PM, Benoit Cousson <b-cousson@ti.com> wrote:
> Replace the regular kzalloc and ioremap with the devm_ equivalent
> to simplify error handling.
>
> Add the missing devm_request_mem_region to reserve the region used
> by the driver.
>
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> ---
>  drivers/gpio/gpio-omap.c |   35 +++++++++++++++--------------------
>  1 files changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index a0c3e03..c3a9dc8 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -19,7 +19,7 @@
>  #include <linux/err.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> -#include <linux/slab.h>
> +#include <linux/device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm.h>
>
> @@ -1052,23 +1052,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>        struct gpio_bank *bank;
>        int ret = 0;
>
> -       if (!dev->platform_data) {
> -               ret = -EINVAL;
> -               goto err_exit;
> -       }
> +       if (!dev->platform_data)
> +               return -EINVAL;
>
> -       bank = kzalloc(sizeof(struct gpio_bank), GFP_KERNEL);
> +       bank = devm_kzalloc(&pdev->dev, sizeof(struct gpio_bank), GFP_KERNEL);
>        if (!bank) {
>                dev_err(dev, "Memory alloc failed\n");
> -               ret = -ENOMEM;
> -               goto err_exit;
> +               return -ENOMEM;
>        }
>
>        res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>        if (unlikely(!res)) {
>                dev_err(dev, "Invalid IRQ resource\n");
> -               ret = -ENODEV;
> -               goto err_free;
> +               return -ENODEV;
How is the memory allocated to 'bank' getting freed before return -ENODEV?

>        }
>
>        bank->irq = res->start;
> @@ -1096,15 +1092,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>        if (unlikely(!res)) {
>                dev_err(dev, "Invalid mem resource\n");
> -               ret = -ENODEV;
> -               goto err_free;
> +               return -ENODEV;
And here?

> +       }
> +
> +       if (!devm_request_mem_region(dev, res->start, resource_size(res),
> +                                    pdev->name)) {
> +               dev_err(dev, "Region already claimed\n");
> +               return -EBUSY;
>        }
>
> -       bank->base = ioremap(res->start, resource_size(res));
> +       bank->base = devm_ioremap(dev, res->start, resource_size(res));
>        if (!bank->base) {
>                dev_err(dev, "Could not ioremap\n");
> -               ret = -ENOMEM;
> -               goto err_free;
> +               return -ENOMEM;
And here?
--
Tarun
>        }
>
>        platform_set_drvdata(pdev, bank);
> @@ -1125,11 +1125,6 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>        list_add_tail(&bank->node, &omap_gpio_list);
>
>        return ret;
> -
> -err_free:
> -       kfree(bank);
> -err_exit:
> -       return ret;
>  }
>
>  #ifdef CONFIG_ARCH_OMAP2PLUS
> --
> 1.7.0.4
>

Re: [PATCH 1/5] gpio/omap: Remove bank->id information and misc cleanup

[DebBarma, Tarun Kanti]

On Wed, Feb 15, 2012 at 9:34 PM, Benoit Cousson <b-cousson@ti.com> wrote:
> The driver does not need anymore any id to identify the GPIO instance.
> Remove every occurence of the bank->id inside the driver.
>
> Remove two trailing spaces.
> Add a dev variable for better readability in probe.
> Remove unused variable bank->pbase.
>
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>
Looks fine.
If needed you can add my Acked-by:
--
Tarun

> ---
>  drivers/gpio/gpio-omap.c |   23 +++++++++--------------
>  1 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index f49bd6f..a0c3e03 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -50,7 +50,6 @@ struct gpio_regs {
>
>  struct gpio_bank {
>        struct list_head node;
> -       unsigned long pbase;
>        void __iomem *base;
>        u16 irq;
>        u16 virtual_irq_start;
> @@ -77,7 +76,6 @@ struct gpio_bank {
>        int stride;
>        u32 width;
>        int context_loss_count;
> -       u16 id;
>        int power_mode;
>        bool workaround_enabled;
>
> @@ -155,7 +153,7 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>  {
>        int l = __raw_readl(base + reg);
>
> -       if (set)
> +       if (set)
>                l |= mask;
>        else
>                l &= ~mask;
> @@ -495,7 +493,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,
>                        "Unable to modify wakeup on non-wakeup GPIO%d\n", gpio);
>                return -EINVAL;
>        }
> @@ -1048,37 +1046,36 @@ static void __devinit omap_gpio_chip_init(struct gpio_bank *bank)
>
>  static int __devinit omap_gpio_probe(struct platform_device *pdev)
>  {
> +       struct device *dev = &pdev->dev;
>        struct omap_gpio_platform_data *pdata;
>        struct resource *res;
>        struct gpio_bank *bank;
>        int ret = 0;
>
> -       if (!pdev->dev.platform_data) {
> +       if (!dev->platform_data) {
>                ret = -EINVAL;
>                goto err_exit;
>        }
>
>        bank = kzalloc(sizeof(struct gpio_bank), GFP_KERNEL);
>        if (!bank) {
> -               dev_err(&pdev->dev, "Memory alloc failed for gpio_bank\n");
> +               dev_err(dev, "Memory alloc failed\n");
>                ret = -ENOMEM;
>                goto err_exit;
>        }
>
>        res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>        if (unlikely(!res)) {
> -               dev_err(&pdev->dev, "GPIO Bank %i Invalid IRQ resource\n",
> -                               pdev->id);
> +               dev_err(dev, "Invalid IRQ resource\n");
>                ret = -ENODEV;
>                goto err_free;
>        }
>
>        bank->irq = res->start;
> -       bank->id = pdev->id;
>
>        pdata = pdev->dev.platform_data;
>        bank->virtual_irq_start = pdata->virtual_irq_start;
> -       bank->dev = &pdev->dev;
> +       bank->dev = dev;
>        bank->dbck_flag = pdata->dbck_flag;
>        bank->stride = pdata->bank_stride;
>        bank->width = pdata->bank_width;
> @@ -1098,16 +1095,14 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>        /* Static mapping, never released */
>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>        if (unlikely(!res)) {
> -               dev_err(&pdev->dev, "GPIO Bank %i Invalid mem resource\n",
> -                               pdev->id);
> +               dev_err(dev, "Invalid mem resource\n");
>                ret = -ENODEV;
>                goto err_free;
>        }
>
>        bank->base = ioremap(res->start, resource_size(res));
>        if (!bank->base) {
> -               dev_err(&pdev->dev, "Could not ioremap gpio bank%i\n",
> -                               pdev->id);
> +               dev_err(dev, "Could not ioremap\n");
>                ret = -ENOMEM;
>                goto err_free;
>        }
> --
> 1.7.0.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] gpio/omap: Use devm_ API and add request_mem_region

[Grant Likely]

On Thu, Feb 16, 2012 at 11:11:52AM +0530, DebBarma, Tarun Kanti wrote:
> Hi Benoit,
> 
> On Wed, Feb 15, 2012 at 9:34 PM, Benoit Cousson <b-cousson@ti.com> wrote:
> > Replace the regular kzalloc and ioremap with the devm_ equivalent
> > to simplify error handling.
> >
> > Add the missing devm_request_mem_region to reserve the region used
> > by the driver.
> >
> > Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> > Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> > ---
> >  drivers/gpio/gpio-omap.c |   35 +++++++++++++++--------------------
> >  1 files changed, 15 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index a0c3e03..c3a9dc8 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -19,7 +19,7 @@
> >  #include <linux/err.h>
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> > -#include <linux/slab.h>
> > +#include <linux/device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/pm.h>
> >
> > @@ -1052,23 +1052,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
> >        struct gpio_bank *bank;
> >        int ret = 0;
> >
> > -       if (!dev->platform_data) {
> > -               ret = -EINVAL;
> > -               goto err_exit;
> > -       }
> > +       if (!dev->platform_data)
> > +               return -EINVAL;
> >
> > -       bank = kzalloc(sizeof(struct gpio_bank), GFP_KERNEL);
> > +       bank = devm_kzalloc(&pdev->dev, sizeof(struct gpio_bank), GFP_KERNEL);
> >        if (!bank) {
> >                dev_err(dev, "Memory alloc failed\n");
> > -               ret = -ENOMEM;
> > -               goto err_exit;
> > +               return -ENOMEM;
> >        }
> >
> >        res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >        if (unlikely(!res)) {
> >                dev_err(dev, "Invalid IRQ resource\n");
> > -               ret = -ENODEV;
> > -               goto err_free;
> > +               return -ENODEV;
> How is the memory allocated to 'bank' getting freed before return -ENODEV?

The magic of devm_*() functions.  Anything allocated by those functions is
automatically freed on either probe failure or driver removal.

g.

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

Re: [PATCH 2/5] gpio/omap: Use devm_ API and add request_mem_region

[Shubhrajyoti]

On Thursday 16 February 2012 11:11 AM, DebBarma, Tarun Kanti wrote:
> Hi Benoit,
>
> On Wed, Feb 15, 2012 at 9:34 PM, Benoit Cousson <b-cousson@ti.com> wrote:
>> Replace the regular kzalloc and ioremap with the devm_ equivalent
>> to simplify error handling.
>>
>> Add the missing devm_request_mem_region to reserve the region used
>> by the driver.
>>
>> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
>> Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>> ---
>>  drivers/gpio/gpio-omap.c |   35 +++++++++++++++--------------------
>>  1 files changed, 15 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index a0c3e03..c3a9dc8 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -19,7 +19,7 @@
>>  #include <linux/err.h>
>>  #include <linux/clk.h>
>>  #include <linux/io.h>
>> -#include <linux/slab.h>
>> +#include <linux/device.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/pm.h>
>>
>> @@ -1052,23 +1052,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>>        struct gpio_bank *bank;
>>        int ret = 0;
>>
>> -       if (!dev->platform_data) {
>> -               ret = -EINVAL;
>> -               goto err_exit;
>> -       }
>> +       if (!dev->platform_data)
>> +               return -EINVAL;
>>
>> -       bank = kzalloc(sizeof(struct gpio_bank), GFP_KERNEL);
>> +       bank = devm_kzalloc(&pdev->dev, sizeof(struct gpio_bank), GFP_KERNEL);
>>        if (!bank) {
>>                dev_err(dev, "Memory alloc failed\n");
>> -               ret = -ENOMEM;
>> -               goto err_exit;
>> +               return -ENOMEM;
>>        }
>>
>>        res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>        if (unlikely(!res)) {
>>                dev_err(dev, "Invalid IRQ resource\n");
>> -               ret = -ENODEV;
>> -               goto err_free;
>> +               return -ENODEV;
> How is the memory allocated to 'bank' getting freed before return -ENODEV?

I think that may not be needed

See http://www.kernel.org/doc/htmldocs/device-drivers/API-devm-kzalloc.html



>>        }
>>
>>        bank->irq = res->start;
>> @@ -1096,15 +1092,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>        if (unlikely(!res)) {
>>                dev_err(dev, "Invalid mem resource\n");
>> -               ret = -ENODEV;
>> -               goto err_free;
>> +               return -ENODEV;
> And here?
>
>> +       }
>> +
>> +       if (!devm_request_mem_region(dev, res->start, resource_size(res),
>> +                                    pdev->name)) {
>> +               dev_err(dev, "Region already claimed\n");
>> +               return -EBUSY;
>>        }
>>
>> -       bank->base = ioremap(res->start, resource_size(res));
>> +       bank->base = devm_ioremap(dev, res->start, resource_size(res));
>>        if (!bank->base) {
>>                dev_err(dev, "Could not ioremap\n");
>> -               ret = -ENOMEM;
>> -               goto err_free;
>> +               return -ENOMEM;
> And here?
> --
> Tarun
>>        }
>>
>>        platform_set_drvdata(pdev, bank);
>> @@ -1125,11 +1125,6 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>>        list_add_tail(&bank->node, &omap_gpio_list);
>>
>>        return ret;
>> -
>> -err_free:
>> -       kfree(bank);
>> -err_exit:
>> -       return ret;
>>  }
>>
>>  #ifdef CONFIG_ARCH_OMAP2PLUS
>> --
>> 1.7.0.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] gpio/omap: Use devm_ API and add request_mem_region

[DebBarma, Tarun Kanti]

On Thu, Feb 16, 2012 at 12:05 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Thu, Feb 16, 2012 at 11:11:52AM +0530, DebBarma, Tarun Kanti wrote:
>> Hi Benoit,
>>
>> On Wed, Feb 15, 2012 at 9:34 PM, Benoit Cousson <b-cousson@ti.com> wrote:
>> > Replace the regular kzalloc and ioremap with the devm_ equivalent
>> > to simplify error handling.
>> >
>> > Add the missing devm_request_mem_region to reserve the region used
>> > by the driver.
>> >
>> > Signed-off-by: Benoit Cousson <b-cousson@ti.com>
>> > Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>> > ---
>> >  drivers/gpio/gpio-omap.c |   35 +++++++++++++++--------------------
>> >  1 files changed, 15 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> > index a0c3e03..c3a9dc8 100644
>> > --- a/drivers/gpio/gpio-omap.c
>> > +++ b/drivers/gpio/gpio-omap.c
>> > @@ -19,7 +19,7 @@
>> >  #include <linux/err.h>
>> >  #include <linux/clk.h>
>> >  #include <linux/io.h>
>> > -#include <linux/slab.h>
>> > +#include <linux/device.h>
>> >  #include <linux/pm_runtime.h>
>> >  #include <linux/pm.h>
>> >
>> > @@ -1052,23 +1052,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>> >        struct gpio_bank *bank;
>> >        int ret = 0;
>> >
>> > -       if (!dev->platform_data) {
>> > -               ret = -EINVAL;
>> > -               goto err_exit;
>> > -       }
>> > +       if (!dev->platform_data)
>> > +               return -EINVAL;
>> >
>> > -       bank = kzalloc(sizeof(struct gpio_bank), GFP_KERNEL);
>> > +       bank = devm_kzalloc(&pdev->dev, sizeof(struct gpio_bank), GFP_KERNEL);
>> >        if (!bank) {
>> >                dev_err(dev, "Memory alloc failed\n");
>> > -               ret = -ENOMEM;
>> > -               goto err_exit;
>> > +               return -ENOMEM;
>> >        }
>> >
>> >        res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> >        if (unlikely(!res)) {
>> >                dev_err(dev, "Invalid IRQ resource\n");
>> > -               ret = -ENODEV;
>> > -               goto err_free;
>> > +               return -ENODEV;
>> How is the memory allocated to 'bank' getting freed before return -ENODEV?
>
> The magic of devm_*() functions.  Anything allocated by those functions is
> automatically freed on either probe failure or driver removal.
OK, thanks.
--
Tarun
>
> g.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] gpio/omap: Use devm_ API and add request_mem_region

[Cousson, Benoit]

On 2/16/2012 7:37 AM, Shubhrajyoti wrote:
> On Thursday 16 February 2012 11:11 AM, DebBarma, Tarun Kanti wrote:
>> Hi Benoit,
>>
>> On Wed, Feb 15, 2012 at 9:34 PM, Benoit Cousson<b-cousson@ti.com>  wrote:
>>> Replace the regular kzalloc and ioremap with the devm_ equivalent
>>> to simplify error handling.
>>>
>>> Add the missing devm_request_mem_region to reserve the region used
>>> by the driver.
>>>
>>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>>> Cc: Tarun Kanti DebBarma<tarun.kanti@ti.com>
>>> ---
>>>   drivers/gpio/gpio-omap.c |   35 +++++++++++++++--------------------
>>>   1 files changed, 15 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index a0c3e03..c3a9dc8 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -19,7 +19,7 @@
>>>   #include<linux/err.h>
>>>   #include<linux/clk.h>
>>>   #include<linux/io.h>
>>> -#include<linux/slab.h>
>>> +#include<linux/device.h>
>>>   #include<linux/pm_runtime.h>
>>>   #include<linux/pm.h>
>>>
>>> @@ -1052,23 +1052,19 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>>>         struct gpio_bank *bank;
>>>         int ret = 0;
>>>
>>> -       if (!dev->platform_data) {
>>> -               ret = -EINVAL;
>>> -               goto err_exit;
>>> -       }
>>> +       if (!dev->platform_data)
>>> +               return -EINVAL;
>>>
>>> -       bank = kzalloc(sizeof(struct gpio_bank), GFP_KERNEL);
>>> +       bank = devm_kzalloc(&pdev->dev, sizeof(struct gpio_bank), GFP_KERNEL);
>>>         if (!bank) {
>>>                 dev_err(dev, "Memory alloc failed\n");
>>> -               ret = -ENOMEM;
>>> -               goto err_exit;
>>> +               return -ENOMEM;
>>>         }
>>>
>>>         res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>         if (unlikely(!res)) {
>>>                 dev_err(dev, "Invalid IRQ resource\n");
>>> -               ret = -ENODEV;
>>> -               goto err_free;
>>> +               return -ENODEV;
>> How is the memory allocated to 'bank' getting freed before return -ENODEV?
>
> I think that may not be needed

It is indeed not needed, and this is the whole point of that API 
compared to the regular kzalloc... and that's why it is highly 
recommended to switch to this kind of API.

Regards,
Benoit

Re: [PATCH 1/5] gpio/omap: Remove bank->id information and misc cleanup

[Cousson, Benoit]

On 2/16/2012 6:53 AM, DebBarma, Tarun Kanti wrote:
> On Wed, Feb 15, 2012 at 9:34 PM, Benoit Cousson<b-cousson@ti.com>  wrote:
>> The driver does not need anymore any id to identify the GPIO instance.
>> Remove every occurence of the bank->id inside the driver.
>>
>> Remove two trailing spaces.
>> Add a dev variable for better readability in probe.
>> Remove unused variable bank->pbase.
>>
>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>> Cc: Tarun Kanti DebBarma<tarun.kanti@ti.com>
> Looks fine.
> If needed you can add my Acked-by:

Thanks,
Benoit

> --
> Tarun
>
>> ---
>>   drivers/gpio/gpio-omap.c |   23 +++++++++--------------
>>   1 files changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index f49bd6f..a0c3e03 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -50,7 +50,6 @@ struct gpio_regs {
>>
>>   struct gpio_bank {
>>         struct list_head node;
>> -       unsigned long pbase;
>>         void __iomem *base;
>>         u16 irq;
>>         u16 virtual_irq_start;
>> @@ -77,7 +76,6 @@ struct gpio_bank {
>>         int stride;
>>         u32 width;
>>         int context_loss_count;
>> -       u16 id;
>>         int power_mode;
>>         bool workaround_enabled;
>>
>> @@ -155,7 +153,7 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>>   {
>>         int l = __raw_readl(base + reg);
>>
>> -       if (set)
>> +       if (set)
>>                 l |= mask;
>>         else
>>                 l&= ~mask;
>> @@ -495,7 +493,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,
>>                         "Unable to modify wakeup on non-wakeup GPIO%d\n", gpio);
>>                 return -EINVAL;
>>         }
>> @@ -1048,37 +1046,36 @@ static void __devinit omap_gpio_chip_init(struct gpio_bank *bank)
>>
>>   static int __devinit omap_gpio_probe(struct platform_device *pdev)
>>   {
>> +       struct device *dev =&pdev->dev;
>>         struct omap_gpio_platform_data *pdata;
>>         struct resource *res;
>>         struct gpio_bank *bank;
>>         int ret = 0;
>>
>> -       if (!pdev->dev.platform_data) {
>> +       if (!dev->platform_data) {
>>                 ret = -EINVAL;
>>                 goto err_exit;
>>         }
>>
>>         bank = kzalloc(sizeof(struct gpio_bank), GFP_KERNEL);
>>         if (!bank) {
>> -               dev_err(&pdev->dev, "Memory alloc failed for gpio_bank\n");
>> +               dev_err(dev, "Memory alloc failed\n");
>>                 ret = -ENOMEM;
>>                 goto err_exit;
>>         }
>>
>>         res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>         if (unlikely(!res)) {
>> -               dev_err(&pdev->dev, "GPIO Bank %i Invalid IRQ resource\n",
>> -                               pdev->id);
>> +               dev_err(dev, "Invalid IRQ resource\n");
>>                 ret = -ENODEV;
>>                 goto err_free;
>>         }
>>
>>         bank->irq = res->start;
>> -       bank->id = pdev->id;
>>
>>         pdata = pdev->dev.platform_data;
>>         bank->virtual_irq_start = pdata->virtual_irq_start;
>> -       bank->dev =&pdev->dev;
>> +       bank->dev = dev;
>>         bank->dbck_flag = pdata->dbck_flag;
>>         bank->stride = pdata->bank_stride;
>>         bank->width = pdata->bank_width;
>> @@ -1098,16 +1095,14 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>>         /* Static mapping, never released */
>>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>         if (unlikely(!res)) {
>> -               dev_err(&pdev->dev, "GPIO Bank %i Invalid mem resource\n",
>> -                               pdev->id);
>> +               dev_err(dev, "Invalid mem resource\n");
>>                 ret = -ENODEV;
>>                 goto err_free;
>>         }
>>
>>         bank->base = ioremap(res->start, resource_size(res));
>>         if (!bank->base) {
>> -               dev_err(&pdev->dev, "Could not ioremap gpio bank%i\n",
>> -                               pdev->id);
>> +               dev_err(dev, "Could not ioremap\n");
>>                 ret = -ENOMEM;
>>                 goto err_free;
>>         }
>> --
>> 1.7.0.4
>>

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Rob Herring]

On 02/15/2012 10:04 AM, Benoit Cousson wrote:
> Adapt the GPIO driver to retrieve information from a DT file.
> 
> Allocate the irq_base dynamically and rename bank->virtual_irq_start
> to bank->irq_base.
> Change irq_base type to int instead of u16 to match irq_alloc_descs
> output.
> 
> Add documentation for GPIO properties specific to OMAP.
> 
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>

One comment below, but otherwise:

Acked-by: Rob Herring <rob.herring@calxeda.com>

> ---
>  .../devicetree/bindings/gpio/gpio-omap.txt         |   30 +++++
>  drivers/gpio/gpio-omap.c                           |  121 ++++++++++++++++++--
>  2 files changed, 142 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..c1b3100
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
> @@ -0,0 +1,30 @@
> +OMAP GPIO controller bindings
> +
> +Required properties:
> +- compatible:
> +  - "ti,omap2-gpio" for OMAP2 controllers
> +  - "ti,omap3-gpio" for OMAP3 controllers
> +  - "ti,omap4-gpio" for OMAP4 controllers
> +- #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.
> +- #interrupt-cells : Should be one

There's no level/edge settings for gpios?

> +- interrupt-controller: Mark the device node as an interrupt controller
> +
> +OMAP specific properties:
> +- ti,hwmods: Name of the hwmod associated to the GPIO:
> +  "gpio<X>", <X> being the 1-based instance number from the HW spec
> +
> +
> +Example:
> +
> +gpio4: gpio4 {
> +    compatible = "ti,omap4-gpio";
> +    ti,hwmods = "gpio4";
> +    #gpio-cells = <2>;
> +    gpio-controller;
> +    #interrupt-cells = <1>;
> +    interrupt-controller;
> +};
> +
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index c3a9dc8..bc2bd69 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -22,6 +22,9 @@
>  #include <linux/device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/irqdomain.h>
>  
>  #include <mach/hardware.h>
>  #include <asm/irq.h>
> @@ -52,7 +55,8 @@ struct gpio_bank {
>  	struct list_head node;
>  	void __iomem *base;
>  	u16 irq;
> -	u16 virtual_irq_start;
> +	int irq_base;
> +	struct irq_domain *domain;
>  	u32 suspend_wakeup;
>  	u32 saved_wakeup;
>  	u32 non_wakeup_gpios;
> @@ -669,7 +673,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>  		if (!isr)
>  			break;
>  
> -		gpio_irq = bank->virtual_irq_start;
> +		gpio_irq = bank->irq_base;
>  		for (; isr != 0; isr >>= 1, gpio_irq++) {
>  			gpio_index = GPIO_INDEX(bank, irq_to_gpio(gpio_irq));
>  
> @@ -915,7 +919,7 @@ static int gpio_2irq(struct gpio_chip *chip, unsigned offset)
>  	struct gpio_bank *bank;
>  
>  	bank = container_of(chip, struct gpio_bank, chip);
> -	return bank->virtual_irq_start + offset;
> +	return bank->irq_base + offset;
>  }
>  
>  /*---------------------------------------------------------------------*/
> @@ -1028,8 +1032,7 @@ static void __devinit omap_gpio_chip_init(struct gpio_bank *bank)
>  
>  	gpiochip_add(&bank->chip);
>  
> -	for (j = bank->virtual_irq_start;
> -		     j < bank->virtual_irq_start + bank->width; j++) {
> +	for (j = bank->irq_base; j < bank->irq_base + bank->width; j++) {
>  		irq_set_lockdep_class(j, &gpio_lock_class);
>  		irq_set_chip_data(j, bank);
>  		if (bank->is_mpuio) {
> @@ -1044,15 +1047,22 @@ 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)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	const struct of_device_id *match;
>  	struct omap_gpio_platform_data *pdata;
>  	struct resource *res;
>  	struct gpio_bank *bank;
>  	int ret = 0;
>  
> -	if (!dev->platform_data)
> +	match = of_match_device(of_match_ptr(omap_gpio_match), dev);
> +
> +	pdata = match ? match->data : dev->platform_data;
> +	if (!pdata)
>  		return -EINVAL;
>  
>  	bank = devm_kzalloc(&pdev->dev, sizeof(struct gpio_bank), GFP_KERNEL);
> @@ -1068,9 +1078,6 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	bank->irq = res->start;
> -
> -	pdata = pdev->dev.platform_data;
> -	bank->virtual_irq_start = pdata->virtual_irq_start;
>  	bank->dev = dev;
>  	bank->dbck_flag = pdata->dbck_flag;
>  	bank->stride = pdata->bank_stride;
> @@ -1080,6 +1087,18 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>  	bank->loses_context = pdata->loses_context;
>  	bank->get_context_loss_count = pdata->get_context_loss_count;
>  	bank->regs = pdata->regs;
> +#ifdef CONFIG_OF_GPIO
> +	bank->chip.of_node = of_node_get(node);
> +#endif
> +
> +	bank->irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
> +	if (bank->irq_base < 0) {
> +		dev_err(dev, "Couldn't allocate IRQ numbers\n");
> +		return -ENODEV;
> +	}
> +
> +	bank->domain = irq_domain_add_legacy(node, bank->width, bank->irq_base,
> +					     0, &irq_domain_simple_ops, NULL);
>  
>  	if (bank->regs->set_dataout && bank->regs->clr_dataout)
>  		bank->set_dataout = _set_gpio_dataout_reg;
> @@ -1387,11 +1406,95 @@ static const struct dev_pm_ops gpio_pm_ops = {
>  									NULL)
>  };
>  
> +#if defined(CONFIG_OF)
> +static struct omap_gpio_reg_offs omap2_gpio_regs = {
> +	.revision =		OMAP24XX_GPIO_REVISION,
> +	.direction =		OMAP24XX_GPIO_OE,
> +	.datain =		OMAP24XX_GPIO_DATAIN,
> +	.dataout =		OMAP24XX_GPIO_DATAOUT,
> +	.set_dataout =		OMAP24XX_GPIO_SETDATAOUT,
> +	.clr_dataout =		OMAP24XX_GPIO_CLEARDATAOUT,
> +	.irqstatus =		OMAP24XX_GPIO_IRQSTATUS1,
> +	.irqstatus2 =		OMAP24XX_GPIO_IRQSTATUS2,
> +	.irqenable =		OMAP24XX_GPIO_IRQENABLE1,
> +	.irqenable2 =		OMAP24XX_GPIO_IRQENABLE2,
> +	.set_irqenable =	OMAP24XX_GPIO_SETIRQENABLE1,
> +	.clr_irqenable =	OMAP24XX_GPIO_CLEARIRQENABLE1,
> +	.debounce =		OMAP24XX_GPIO_DEBOUNCE_VAL,
> +	.debounce_en =		OMAP24XX_GPIO_DEBOUNCE_EN,
> +	.ctrl =			OMAP24XX_GPIO_CTRL,
> +	.wkup_en =		OMAP24XX_GPIO_WAKE_EN,
> +	.leveldetect0 =		OMAP24XX_GPIO_LEVELDETECT0,
> +	.leveldetect1 =		OMAP24XX_GPIO_LEVELDETECT1,
> +	.risingdetect =		OMAP24XX_GPIO_RISINGDETECT,
> +	.fallingdetect =	OMAP24XX_GPIO_FALLINGDETECT,
> +};
> +
> +static struct omap_gpio_reg_offs omap4_gpio_regs = {
> +	.revision =		OMAP4_GPIO_REVISION,
> +	.direction =		OMAP4_GPIO_OE,
> +	.datain =		OMAP4_GPIO_DATAIN,
> +	.dataout =		OMAP4_GPIO_DATAOUT,
> +	.set_dataout =		OMAP4_GPIO_SETDATAOUT,
> +	.clr_dataout =		OMAP4_GPIO_CLEARDATAOUT,
> +	.irqstatus =		OMAP4_GPIO_IRQSTATUS0,
> +	.irqstatus2 =		OMAP4_GPIO_IRQSTATUS1,
> +	.irqenable =		OMAP4_GPIO_IRQSTATUSSET0,
> +	.irqenable2 =		OMAP4_GPIO_IRQSTATUSSET1,
> +	.set_irqenable =	OMAP4_GPIO_IRQSTATUSSET0,
> +	.clr_irqenable =	OMAP4_GPIO_IRQSTATUSCLR0,
> +	.debounce =		OMAP4_GPIO_DEBOUNCINGTIME,
> +	.debounce_en =		OMAP4_GPIO_DEBOUNCENABLE,
> +	.ctrl =			OMAP4_GPIO_CTRL,
> +	.wkup_en =		OMAP4_GPIO_IRQWAKEN0,
> +	.leveldetect0 =		OMAP4_GPIO_LEVELDETECT0,
> +	.leveldetect1 =		OMAP4_GPIO_LEVELDETECT1,
> +	.risingdetect =		OMAP4_GPIO_RISINGDETECT,
> +	.fallingdetect =	OMAP4_GPIO_FALLINGDETECT,
> +};
> +
> +static struct omap_gpio_platform_data omap2_pdata = {
> +	.regs = &omap2_gpio_regs,
> +	.bank_width = 32,
> +	.dbck_flag = false,
> +};
> +
> +static struct omap_gpio_platform_data omap3_pdata = {
> +	.regs = &omap2_gpio_regs,
> +	.bank_width = 32,
> +	.dbck_flag = true,
> +};
> +
> +static struct omap_gpio_platform_data omap4_pdata = {
> +	.regs = &omap4_gpio_regs,
> +	.bank_width = 32,
> +	.dbck_flag = true,
> +};
> +
> +static const struct of_device_id omap_gpio_match[] = {
> +	{
> +		.compatible = "ti,omap4-gpio",
> +		.data = &omap4_pdata,
> +	},
> +	{
> +		.compatible = "ti,omap3-gpio",
> +		.data = &omap3_pdata,
> +	},
> +	{
> +		.compatible = "ti,omap2-gpio",
> +		.data = &omap2_pdata,
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, omap_gpio_match);
> +#endif
> +
>  static struct platform_driver omap_gpio_driver = {
>  	.probe		= omap_gpio_probe,
>  	.driver		= {
>  		.name	= "omap_gpio",
>  		.pm	= &gpio_pm_ops,
> +		.of_match_table = of_match_ptr(omap_gpio_match),
>  	},
>  };
>  

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Cousson, Benoit]

On 2/22/2012 3:23 PM, Rob Herring wrote:
> On 02/15/2012 10:04 AM, Benoit Cousson wrote:
>> Adapt the GPIO driver to retrieve information from a DT file.
>>
>> Allocate the irq_base dynamically and rename bank->virtual_irq_start
>> to bank->irq_base.
>> Change irq_base type to int instead of u16 to match irq_alloc_descs
>> output.
>>
>> Add documentation for GPIO properties specific to OMAP.
>>
>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>> Cc: Tarun Kanti DebBarma<tarun.kanti@ti.com>
>
> One comment below, but otherwise:
>
> Acked-by: Rob Herring<rob.herring@calxeda.com>
>
>> ---
>>   .../devicetree/bindings/gpio/gpio-omap.txt         |   30 +++++
>>   drivers/gpio/gpio-omap.c                           |  121 ++++++++++++++++++--
>>   2 files changed, 142 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..c1b3100
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
>> @@ -0,0 +1,30 @@
>> +OMAP GPIO controller bindings
>> +
>> +Required properties:
>> +- compatible:
>> +  - "ti,omap2-gpio" for OMAP2 controllers
>> +  - "ti,omap3-gpio" for OMAP3 controllers
>> +  - "ti,omap4-gpio" for OMAP4 controllers
>> +- #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.
>> +- #interrupt-cells : Should be one
>
> There's no level/edge settings for gpios?

That's a good question, because I was wondering as well :-)

I did no see how it was done in other GPIO implementation.

Benoit

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Rob Herring]

On 02/22/2012 08:31 AM, Cousson, Benoit wrote:
> On 2/22/2012 3:23 PM, Rob Herring wrote:
>> On 02/15/2012 10:04 AM, Benoit Cousson wrote:
>>> Adapt the GPIO driver to retrieve information from a DT file.
>>>
>>> Allocate the irq_base dynamically and rename bank->virtual_irq_start
>>> to bank->irq_base.
>>> Change irq_base type to int instead of u16 to match irq_alloc_descs
>>> output.
>>>
>>> Add documentation for GPIO properties specific to OMAP.
>>>
>>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>>> Cc: Tarun Kanti DebBarma<tarun.kanti@ti.com>
>>
>> One comment below, but otherwise:
>>
>> Acked-by: Rob Herring<rob.herring@calxeda.com>
>>
>>> ---
>>>   .../devicetree/bindings/gpio/gpio-omap.txt         |   30 +++++
>>>   drivers/gpio/gpio-omap.c                           |  121
>>> ++++++++++++++++++--
>>>   2 files changed, 142 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..c1b3100
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
>>> @@ -0,0 +1,30 @@
>>> +OMAP GPIO controller bindings
>>> +
>>> +Required properties:
>>> +- compatible:
>>> +  - "ti,omap2-gpio" for OMAP2 controllers
>>> +  - "ti,omap3-gpio" for OMAP3 controllers
>>> +  - "ti,omap4-gpio" for OMAP4 controllers
>>> +- #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.
>>> +- #interrupt-cells : Should be one
>>
>> There's no level/edge settings for gpios?
> 
> That's a good question, because I was wondering as well :-)
> 
> I did no see how it was done in other GPIO implementation.

There's not really a good example that I've found. Many gpio nodes don't
even have "interrupt-controller" set.

So if you have an irq_set_type function for gpio's, then you should have
2 cells.

Rob

RE: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

Rob Herring wrote at Wednesday, February 22, 2012 10:23 AM:
> On 02/22/2012 08:31 AM, Cousson, Benoit wrote:
> > On 2/22/2012 3:23 PM, Rob Herring wrote:
> >> On 02/15/2012 10:04 AM, Benoit Cousson wrote:
> >>> Adapt the GPIO driver to retrieve information from a DT file.
> >>>
> >>> Allocate the irq_base dynamically and rename bank->virtual_irq_start
> >>> to bank->irq_base.
> >>> Change irq_base type to int instead of u16 to match irq_alloc_descs
> >>> output.
> >>>
> >>> Add documentation for GPIO properties specific to OMAP.
> >>>
> >>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
> >>> Cc: Tarun Kanti DebBarma<tarun.kanti@ti.com>
> >>
> >> One comment below, but otherwise:
> >>
> >> Acked-by: Rob Herring<rob.herring@calxeda.com>
> >>
> >>> ---
> >>>   .../devicetree/bindings/gpio/gpio-omap.txt         |   30 +++++
> >>>   drivers/gpio/gpio-omap.c                           |  121
> >>> ++++++++++++++++++--
> >>>   2 files changed, 142 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..c1b3100
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
> >>> @@ -0,0 +1,30 @@
> >>> +OMAP GPIO controller bindings
> >>> +
> >>> +Required properties:
> >>> +- compatible:
> >>> +  - "ti,omap2-gpio" for OMAP2 controllers
> >>> +  - "ti,omap3-gpio" for OMAP3 controllers
> >>> +  - "ti,omap4-gpio" for OMAP4 controllers
> >>> +- #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.
> >>> +- #interrupt-cells : Should be one
> >>
> >> There's no level/edge settings for gpios?
> >
> > That's a good question, because I was wondering as well :-)
> >
> > I did no see how it was done in other GPIO implementation.
> 
> There's not really a good example that I've found. Many gpio nodes don't
> even have "interrupt-controller" set.
> 
> So if you have an irq_set_type function for gpio's, then you should have
> 2 cells.

Tegra's GPIO IRQ binding (gpio_nvidia.txt in linux-next at least) says:

- #interrupt-cells : Should be 2.
  The first cell is the GPIO number.
  The second cell is used to specify flags:
    bits[3:0] trigger type and level flags:
      1 = low-to-high edge triggered.
      2 = high-to-low edge triggered.
      4 = active high level-sensitive.
      8 = active low level-sensitive.
      Valid combinations are 1, 2, 3, 4, 8.

Presumably, that's what you meant.

-- 
nvpublic

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Cousson, Benoit]

On 2/22/2012 7:29 PM, Stephen Warren wrote:
> Rob Herring wrote at Wednesday, February 22, 2012 10:23 AM:
>> On 02/22/2012 08:31 AM, Cousson, Benoit wrote:
>>> On 2/22/2012 3:23 PM, Rob Herring wrote:
>>>> On 02/15/2012 10:04 AM, Benoit Cousson wrote:
>>>>> Adapt the GPIO driver to retrieve information from a DT file.
>>>>>
>>>>> Allocate the irq_base dynamically and rename bank->virtual_irq_start
>>>>> to bank->irq_base.
>>>>> Change irq_base type to int instead of u16 to match irq_alloc_descs
>>>>> output.
>>>>>
>>>>> Add documentation for GPIO properties specific to OMAP.
>>>>>
>>>>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>>>>> Cc: Tarun Kanti DebBarma<tarun.kanti@ti.com>
>>>>
>>>> One comment below, but otherwise:
>>>>
>>>> Acked-by: Rob Herring<rob.herring@calxeda.com>
>>>>
>>>>> ---
>>>>>    .../devicetree/bindings/gpio/gpio-omap.txt         |   30 +++++
>>>>>    drivers/gpio/gpio-omap.c                           |  121
>>>>> ++++++++++++++++++--
>>>>>    2 files changed, 142 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..c1b3100
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
>>>>> @@ -0,0 +1,30 @@
>>>>> +OMAP GPIO controller bindings
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible:
>>>>> +  - "ti,omap2-gpio" for OMAP2 controllers
>>>>> +  - "ti,omap3-gpio" for OMAP3 controllers
>>>>> +  - "ti,omap4-gpio" for OMAP4 controllers
>>>>> +- #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.
>>>>> +- #interrupt-cells : Should be one
>>>>
>>>> There's no level/edge settings for gpios?
>>>
>>> That's a good question, because I was wondering as well :-)
>>>
>>> I did no see how it was done in other GPIO implementation.
>>
>> There's not really a good example that I've found. Many gpio nodes don't
>> even have "interrupt-controller" set.
>>
>> So if you have an irq_set_type function for gpio's, then you should have
>> 2 cells.

After checking the OMAP gpio code, I do have a gpio_irq_type and the 
GPIO do support the 4 types. I'm not sure yet about the combinations.

> Tegra's GPIO IRQ binding (gpio_nvidia.txt in linux-next at least) says:
>
> - #interrupt-cells : Should be 2.
>    The first cell is the GPIO number.
>    The second cell is used to specify flags:
>      bits[3:0] trigger type and level flags:
>        1 = low-to-high edge triggered.
>        2 = high-to-low edge triggered.
>        4 = active high level-sensitive.
>        8 = active low level-sensitive.
>        Valid combinations are 1, 2, 3, 4, 8.

Indeed, so I'll just copy / paste Tegra's binding...

Thanks,
Benoit

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Fri, Feb 24, 2012 at 4:30 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 2/22/2012 7:29 PM, Stephen Warren wrote:
>>
>> Rob Herring wrote at Wednesday, February 22, 2012 10:23 AM:
>>>
>>> On 02/22/2012 08:31 AM, Cousson, Benoit wrote:
>>>>
>>>> On 2/22/2012 3:23 PM, Rob Herring wrote:
>>>>>
>>>>> On 02/15/2012 10:04 AM, Benoit Cousson wrote:
>>>>>>
>>>>>> Adapt the GPIO driver to retrieve information from a DT file.
>>>>>>
>>>>>> Allocate the irq_base dynamically and rename bank->virtual_irq_start
>>>>>> to bank->irq_base.
>>>>>> Change irq_base type to int instead of u16 to match irq_alloc_descs
>>>>>> output.
>>>>>>
>>>>>> Add documentation for GPIO properties specific to OMAP.
>>>>>>
>>>>>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>>>>>> Cc: Tarun Kanti DebBarma<tarun.kanti@ti.com>
>>>>>
>>>>>
>>>>> One comment below, but otherwise:
>>>>>
>>>>> Acked-by: Rob Herring<rob.herring@calxeda.com>
>>>>>
>>>>>> ---
>>>>>>    .../devicetree/bindings/gpio/gpio-omap.txt         |   30 +++++
>>>>>>    drivers/gpio/gpio-omap.c                           |  121
>>>>>> ++++++++++++++++++--
>>>>>>    2 files changed, 142 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..c1b3100
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
>>>>>> @@ -0,0 +1,30 @@
>>>>>> +OMAP GPIO controller bindings
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible:
>>>>>> +  - "ti,omap2-gpio" for OMAP2 controllers
>>>>>> +  - "ti,omap3-gpio" for OMAP3 controllers
>>>>>> +  - "ti,omap4-gpio" for OMAP4 controllers
>>>>>> +- #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.
>>>>>> +- #interrupt-cells : Should be one
>>>>>
>>>>>
>>>>> There's no level/edge settings for gpios?
>>>>
>>>>
>>>> That's a good question, because I was wondering as well :-)
>>>>
>>>> I did no see how it was done in other GPIO implementation.
>>>
>>>
>>> There's not really a good example that I've found. Many gpio nodes don't
>>> even have "interrupt-controller" set.
>>>
>>> So if you have an irq_set_type function for gpio's, then you should have
>>> 2 cells.
>
>
> After checking the OMAP gpio code, I do have a gpio_irq_type and the GPIO do
> support the 4 types. I'm not sure yet about the combinations.
>
>
>> Tegra's GPIO IRQ binding (gpio_nvidia.txt in linux-next at least) says:
>>
>> - #interrupt-cells : Should be 2.
>>    The first cell is the GPIO number.
>>    The second cell is used to specify flags:
>>      bits[3:0] trigger type and level flags:
>>        1 = low-to-high edge triggered.
>>        2 = high-to-low edge triggered.
>>        4 = active high level-sensitive.
>>        8 = active low level-sensitive.
>>        Valid combinations are 1, 2, 3, 4, 8.
>
>
> Indeed, so I'll just copy / paste Tegra's binding...
>
> Thanks,
> Benoit
>
>

Hello,

I was wondering if the level/edge settings for gpios is working on OMAP.

I'm adding DT support for an SMSC911x ethernet chip connected to the
GPMC for an OMAP3 SoC based board.

In the smsc911x driver probe function (smsc911x_drv_probe() in
drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.

Reading the gpio-omap.txt documentation it says that #interrupt-cells
should be <2> and that a value of 8 is "active low level-sensitive".

So I tried this:

&gpmc {
	ethernet@5,0 {
		pinctrl-names = "default";
		pinctrl-0 = <&smsc911x_pins>;
		compatible = "smsc,lan9221", "smsc,lan9115";
		reg = <5 0 0xff>; /* CS5 */
		interrupt-parent = <&gpio6>;
		interrupts = <16 8>; /* gpio line 176 */
		interrupt-names = "smsc911x irq";
		vmmc-supply = <&vddvario>;
		vmmc_aux-supply = <&vdd33a>;
		reg-io-width = <4>;

		smsc,save-mac-address;
      };
};

But in the smsc911x probe function:

irq_res->flags & IRQF_TRIGGER_MASK;

returns 0 which means that no trigger flags where set.

I took a look to the GPIOs device node definition on omap{3,4,5}.dtsi
and all look like this:

e.g from omap3.dtsi:

                gpio6: gpio@49058000 {
                        compatible = "ti,omap3-gpio";
                        ti,hwmods = "gpio6";
                        gpio-controller;
                        #gpio-cells = <2>;
                        interrupt-controller;
                        #interrupt-cells = <1>;
                };

So, even when the documentation says that all the GPIO device nodes in
OMAP2+ should use a #interrupt-cells property value of <2>, they are
only using <1>. Changing that value to 2 makes hangs the kernel and it
does not boot.

This is not part of my dayjob and I'm working on this in my free time
so it would be great if someone can give me some pointers or at least
confirm that  this is a known issue so I can debug/fix/extend
drivers/gpio/gpio-omap.c to parse and pass the IRQF_TRIGGER_*  flags
to the IORESOURCE_IRQ resource structure.

Thanks a lot and best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 02/26/2013 03:01 AM, Javier Martinez Canillas wrote:
...
> I was wondering if the level/edge settings for gpios is working on OMAP.
...
> Reading the gpio-omap.txt documentation it says that #interrupt-cells
> should be <2> and that a value of 8 is "active low level-sensitive".
> 
> So I tried this:
> 
> &gpmc {
> 	ethernet@5,0 {
...
> 		interrupt-parent = <&gpio6>;
> 		interrupts = <16 8>; /* gpio line 176 */
> 		interrupt-names = "smsc911x irq";
...
> 
> But in the smsc911x probe function:
> 
> irq_res->flags & IRQF_TRIGGER_MASK;
> 
> returns 0 which means that no trigger flags where set.
> 
> I took a look to the GPIOs device node definition on omap{3,4,5}.dtsi
> and all look like this:
> 
> e.g from omap3.dtsi:
> 
>                 gpio6: gpio@49058000 {
...
>                         #interrupt-cells = <1>;
...
> So, even when the documentation says that all the GPIO device nodes in
> OMAP2+ should use a #interrupt-cells property value of <2>, they are
> only using <1>. Changing that value to 2 makes hangs the kernel and it
> does not boot.

Take a look at what the OMAP GPIO driver is plugging in for the of_xlate
function; does it correctly handle the case where there are two cells?

Is there a default for the flags when no second cell is provided, such
that when you set #interrupt-cells to <2>, you're changing away from the
default and mis-configuring things?

And, did you check for any other interrupts hooked through the GPIO
module, for which you'll need to add the second cell into their GPIO
specifier throughout the entire device tree?

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:

[snip]

> I was wondering if the level/edge settings for gpios is working on OMAP.
> 
> I'm adding DT support for an SMSC911x ethernet chip connected to the
> GPMC for an OMAP3 SoC based board.
> 
> In the smsc911x driver probe function (smsc911x_drv_probe() in
> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
> 
> Reading the gpio-omap.txt documentation it says that #interrupt-cells
> should be <2> and that a value of 8 is "active low level-sensitive".
> 
> So I tried this:
> 
> &gpmc {
> 	ethernet@5,0 {
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&smsc911x_pins>;
> 		compatible = "smsc,lan9221", "smsc,lan9115";
> 		reg = <5 0 0xff>; /* CS5 */
> 		interrupt-parent = <&gpio6>;
> 		interrupts = <16 8>; /* gpio line 176 */
> 		interrupt-names = "smsc911x irq";
> 		vmmc-supply = <&vddvario>;
> 		vmmc_aux-supply = <&vdd33a>;
> 		reg-io-width = <4>;
> 
> 		smsc,save-mac-address;
>       };
> };

Are you requesting the gpio anywhere? If not then this is not going to
work as-is. This was discussed fairly recently [1] and the conclusion
was that the gpio needs to be requested before we can use as an interrupt.

I have not seen your latest smsc code for omap, but when you are
requesting the gpmc chip-select you should also request the gpio.

> But in the smsc911x probe function:
> 
> irq_res->flags & IRQF_TRIGGER_MASK;
> 
> returns 0 which means that no trigger flags where set.
> 
> I took a look to the GPIOs device node definition on omap{3,4,5}.dtsi
> and all look like this:
> 
> e.g from omap3.dtsi:
> 
>                 gpio6: gpio@49058000 {
>                         compatible = "ti,omap3-gpio";
>                         ti,hwmods = "gpio6";
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-controller;
>                         #interrupt-cells = <1>;
>                 };
> 
> So, even when the documentation says that all the GPIO device nodes in
> OMAP2+ should use a #interrupt-cells property value of <2>, they are
> only using <1>. Changing that value to 2 makes hangs the kernel and it
> does not boot.

I will need to take a look at that. Is your code available anywhere so I
can test?

Cheers
Jon

[1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/92192

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 02/26/2013 03:40 PM, Jon Hunter wrote:
> 
> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
> 
> [snip]
> 
>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>
>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>> GPMC for an OMAP3 SoC based board.
>>
>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>
>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>> should be <2> and that a value of 8 is "active low level-sensitive".
>>
>> So I tried this:
>>
>> &gpmc {
>> 	ethernet@5,0 {
>> 		pinctrl-names = "default";
>> 		pinctrl-0 = <&smsc911x_pins>;
>> 		compatible = "smsc,lan9221", "smsc,lan9115";
>> 		reg = <5 0 0xff>; /* CS5 */
>> 		interrupt-parent = <&gpio6>;
>> 		interrupts = <16 8>; /* gpio line 176 */
>> 		interrupt-names = "smsc911x irq";
>> 		vmmc-supply = <&vddvario>;
>> 		vmmc_aux-supply = <&vdd33a>;
>> 		reg-io-width = <4>;
>>
>> 		smsc,save-mac-address;
>>       };
>> };
> 
> Are you requesting the gpio anywhere? If not then this is not going to
> work as-is. This was discussed fairly recently [1] and the conclusion
> was that the gpio needs to be requested before we can use as an interrupt.

That seems wrong; the GPIO/IRQ driver should handle this internally. The
Ethernet driver shouldn't know/care whether the interrupt it's given is
some form of dedicated interrupt or a GPIO-based interrupt, and even if
it somehow did, there's no irq_to_gpio() any more, so the driver can't
tell which GPIO ID it should request, unless it's given yet another
property to represent this.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 02/26/2013 04:44 PM, Stephen Warren wrote:
> On 02/26/2013 03:40 PM, Jon Hunter wrote:
>>
>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>
>> [snip]
>>
>>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>>
>>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>>> GPMC for an OMAP3 SoC based board.
>>>
>>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>>
>>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>>> should be <2> and that a value of 8 is "active low level-sensitive".
>>>
>>> So I tried this:
>>>
>>> &gpmc {
>>> 	ethernet@5,0 {
>>> 		pinctrl-names = "default";
>>> 		pinctrl-0 = <&smsc911x_pins>;
>>> 		compatible = "smsc,lan9221", "smsc,lan9115";
>>> 		reg = <5 0 0xff>; /* CS5 */
>>> 		interrupt-parent = <&gpio6>;
>>> 		interrupts = <16 8>; /* gpio line 176 */
>>> 		interrupt-names = "smsc911x irq";
>>> 		vmmc-supply = <&vddvario>;
>>> 		vmmc_aux-supply = <&vdd33a>;
>>> 		reg-io-width = <4>;
>>>
>>> 		smsc,save-mac-address;
>>>       };
>>> };
>>
>> Are you requesting the gpio anywhere? If not then this is not going to
>> work as-is. This was discussed fairly recently [1] and the conclusion
>> was that the gpio needs to be requested before we can use as an interrupt.
> 
> That seems wrong; the GPIO/IRQ driver should handle this internally. The
> Ethernet driver shouldn't know/care whether the interrupt it's given is
> some form of dedicated interrupt or a GPIO-based interrupt, and even if
> it somehow did, there's no irq_to_gpio() any more, so the driver can't
> tell which GPIO ID it should request, unless it's given yet another
> property to represent this.

I agree that ideally this should be handled internally. Did you read the
discussion on the thread that I referenced [1]? If you have any thoughts
we are open to ideas :-)

Cheers
Jon

[1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 02/26/2013 04:01 PM, Jon Hunter wrote:
> 
> On 02/26/2013 04:44 PM, Stephen Warren wrote:
>> On 02/26/2013 03:40 PM, Jon Hunter wrote:
>>>
>>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>>
>>> [snip]
>>>
>>>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>>>
>>>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>>>> GPMC for an OMAP3 SoC based board.
>>>>
>>>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>>>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>>>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>>>
>>>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>>>> should be <2> and that a value of 8 is "active low level-sensitive".
>>>>
>>>> So I tried this:
>>>>
>>>> &gpmc {
>>>> 	ethernet@5,0 {
>>>> 		pinctrl-names = "default";
>>>> 		pinctrl-0 = <&smsc911x_pins>;
>>>> 		compatible = "smsc,lan9221", "smsc,lan9115";
>>>> 		reg = <5 0 0xff>; /* CS5 */
>>>> 		interrupt-parent = <&gpio6>;
>>>> 		interrupts = <16 8>; /* gpio line 176 */
>>>> 		interrupt-names = "smsc911x irq";
>>>> 		vmmc-supply = <&vddvario>;
>>>> 		vmmc_aux-supply = <&vdd33a>;
>>>> 		reg-io-width = <4>;
>>>>
>>>> 		smsc,save-mac-address;
>>>>       };
>>>> };
>>>
>>> Are you requesting the gpio anywhere? If not then this is not going to
>>> work as-is. This was discussed fairly recently [1] and the conclusion
>>> was that the gpio needs to be requested before we can use as an interrupt.
>>
>> That seems wrong; the GPIO/IRQ driver should handle this internally. The
>> Ethernet driver shouldn't know/care whether the interrupt it's given is
>> some form of dedicated interrupt or a GPIO-based interrupt, and even if
>> it somehow did, there's no irq_to_gpio() any more, so the driver can't
>> tell which GPIO ID it should request, unless it's given yet another
>> property to represent this.
> 
> I agree that ideally this should be handled internally. Did you read the
> discussion on the thread that I referenced [1]? If you have any thoughts
> we are open to ideas :-)
> 
> Cheers
> Jon
> 
> [1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192

Oh, when I clicked that link the first time, all I saw was the patch,
not any discussion. I guess it must have timed out finding the other
emails or something.

I disagree that the GPIO needs to be requested, and that a custom DT
property and associated code are needed for that; simply requesting the
IRQ should be enough to make everything work.

In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
goes and configures the base GPIO HW to enable the pin as a GPIO, just
like gpio_request() would. I imagine the OMAP driver can do whatever
similar action it needs.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:

[snip]

> I was wondering if the level/edge settings for gpios is working on OMAP.
> 
> I'm adding DT support for an SMSC911x ethernet chip connected to the
> GPMC for an OMAP3 SoC based board.
> 
> In the smsc911x driver probe function (smsc911x_drv_probe() in
> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
> 
> Reading the gpio-omap.txt documentation it says that #interrupt-cells
> should be <2> and that a value of 8 is "active low level-sensitive".
> 
> So I tried this:
> 
> &gpmc {
> 	ethernet@5,0 {
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&smsc911x_pins>;
> 		compatible = "smsc,lan9221", "smsc,lan9115";
> 		reg = <5 0 0xff>; /* CS5 */
> 		interrupt-parent = <&gpio6>;
> 		interrupts = <16 8>; /* gpio line 176 */
> 		interrupt-names = "smsc911x irq";
> 		vmmc-supply = <&vddvario>;
> 		vmmc_aux-supply = <&vdd33a>;
> 		reg-io-width = <4>;

By the way, reg-io-width for omap does not look correct. The GPMC only
supports 8-bit or 16-bit devices IIRC. I believe all my omap boards use
16-bit.

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 02/26/2013 05:06 PM, Stephen Warren wrote:
> On 02/26/2013 04:01 PM, Jon Hunter wrote:
>>
>> On 02/26/2013 04:44 PM, Stephen Warren wrote:
>>> On 02/26/2013 03:40 PM, Jon Hunter wrote:
>>>>
>>>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>>>
>>>> [snip]
>>>>
>>>>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>>>>
>>>>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>>>>> GPMC for an OMAP3 SoC based board.
>>>>>
>>>>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>>>>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>>>>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>>>>
>>>>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>>>>> should be <2> and that a value of 8 is "active low level-sensitive".
>>>>>
>>>>> So I tried this:
>>>>>
>>>>> &gpmc {
>>>>> 	ethernet@5,0 {
>>>>> 		pinctrl-names = "default";
>>>>> 		pinctrl-0 = <&smsc911x_pins>;
>>>>> 		compatible = "smsc,lan9221", "smsc,lan9115";
>>>>> 		reg = <5 0 0xff>; /* CS5 */
>>>>> 		interrupt-parent = <&gpio6>;
>>>>> 		interrupts = <16 8>; /* gpio line 176 */
>>>>> 		interrupt-names = "smsc911x irq";
>>>>> 		vmmc-supply = <&vddvario>;
>>>>> 		vmmc_aux-supply = <&vdd33a>;
>>>>> 		reg-io-width = <4>;
>>>>>
>>>>> 		smsc,save-mac-address;
>>>>>       };
>>>>> };
>>>>
>>>> Are you requesting the gpio anywhere? If not then this is not going to
>>>> work as-is. This was discussed fairly recently [1] and the conclusion
>>>> was that the gpio needs to be requested before we can use as an interrupt.
>>>
>>> That seems wrong; the GPIO/IRQ driver should handle this internally. The
>>> Ethernet driver shouldn't know/care whether the interrupt it's given is
>>> some form of dedicated interrupt or a GPIO-based interrupt, and even if
>>> it somehow did, there's no irq_to_gpio() any more, so the driver can't
>>> tell which GPIO ID it should request, unless it's given yet another
>>> property to represent this.
>>
>> I agree that ideally this should be handled internally. Did you read the
>> discussion on the thread that I referenced [1]? If you have any thoughts
>> we are open to ideas :-)
>>
>> Cheers
>> Jon
>>
>> [1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
> 
> Oh, when I clicked that link the first time, all I saw was the patch,
> not any discussion. I guess it must have timed out finding the other
> emails or something.

Actually, I sent a slightly different link the 2nd time to ensure you
saw the thread. So my fault ;-)

> I disagree that the GPIO needs to be requested, and that a custom DT
> property and associated code are needed for that; simply requesting the
> IRQ should be enough to make everything work.
> 
> In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
> goes and configures the base GPIO HW to enable the pin as a GPIO, just
> like gpio_request() would. I imagine the OMAP driver can do whatever
> similar action it needs.

Yes that is similar to what the patch in the thread was attempting to
do, but this got shot down.

One issue I see is that by not calling gpio_request, then potentially
you could have someone request a gpio via gpio_request() and someone
trying to use it as an interrupt source via request_irq(). Now obviously
that represents a bug because there is only one physical gpio, but I
gather it is something we need to protect against.

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 02/26/2013 04:45 PM, Jon Hunter wrote:
> 
> On 02/26/2013 05:06 PM, Stephen Warren wrote:
>> On 02/26/2013 04:01 PM, Jon Hunter wrote:
>>>
>>> On 02/26/2013 04:44 PM, Stephen Warren wrote:
>>>> On 02/26/2013 03:40 PM, Jon Hunter wrote:
>>>>>
>>>>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>>>>
>>>>> [snip]
>>>>>
>>>>>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>>>>>
>>>>>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>>>>>> GPMC for an OMAP3 SoC based board.
>>>>>>
>>>>>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>>>>>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>>>>>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>>>>>
>>>>>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>>>>>> should be <2> and that a value of 8 is "active low level-sensitive".
>>>>>>
>>>>>> So I tried this:
>>>>>>
>>>>>> &gpmc {
>>>>>> 	ethernet@5,0 {
>>>>>> 		pinctrl-names = "default";
>>>>>> 		pinctrl-0 = <&smsc911x_pins>;
>>>>>> 		compatible = "smsc,lan9221", "smsc,lan9115";
>>>>>> 		reg = <5 0 0xff>; /* CS5 */
>>>>>> 		interrupt-parent = <&gpio6>;
>>>>>> 		interrupts = <16 8>; /* gpio line 176 */
>>>>>> 		interrupt-names = "smsc911x irq";
>>>>>> 		vmmc-supply = <&vddvario>;
>>>>>> 		vmmc_aux-supply = <&vdd33a>;
>>>>>> 		reg-io-width = <4>;
>>>>>>
>>>>>> 		smsc,save-mac-address;
>>>>>>       };
>>>>>> };
>>>>>
>>>>> Are you requesting the gpio anywhere? If not then this is not going to
>>>>> work as-is. This was discussed fairly recently [1] and the conclusion
>>>>> was that the gpio needs to be requested before we can use as an interrupt.
>>>>
>>>> That seems wrong; the GPIO/IRQ driver should handle this internally. The
>>>> Ethernet driver shouldn't know/care whether the interrupt it's given is
>>>> some form of dedicated interrupt or a GPIO-based interrupt, and even if
>>>> it somehow did, there's no irq_to_gpio() any more, so the driver can't
>>>> tell which GPIO ID it should request, unless it's given yet another
>>>> property to represent this.
>>>
>>> I agree that ideally this should be handled internally. Did you read the
>>> discussion on the thread that I referenced [1]? If you have any thoughts
>>> we are open to ideas :-)
>>>
>>> Cheers
>>> Jon
>>>
>>> [1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
>>
>> Oh, when I clicked that link the first time, all I saw was the patch,
>> not any discussion. I guess it must have timed out finding the other
>> emails or something.
> 
> Actually, I sent a slightly different link the 2nd time to ensure you
> saw the thread. So my fault ;-)
> 
>> I disagree that the GPIO needs to be requested, and that a custom DT
>> property and associated code are needed for that; simply requesting the
>> IRQ should be enough to make everything work.
>>
>> In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
>> goes and configures the base GPIO HW to enable the pin as a GPIO, just
>> like gpio_request() would. I imagine the OMAP driver can do whatever
>> similar action it needs.
> 
> Yes that is similar to what the patch in the thread was attempting to
> do, but this got shot down.
> 
> One issue I see is that by not calling gpio_request, then potentially
> you could have someone request a gpio via gpio_request() and someone
> trying to use it as an interrupt source via request_irq(). Now obviously
> that represents a bug because there is only one physical gpio, but I
> gather it is something we need to protect against.

I'm not sure it's really that much of an issue, but presumably the
solution is for a combined GPIO+IRQ driver to simply call gpio_request
internally from within some irq_chip function. It looks like struct
irq_chip doesn't have a request/free, but perhaps they could be added to
solve this?

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 02/26/2013 06:13 PM, Stephen Warren wrote:
> On 02/26/2013 04:45 PM, Jon Hunter wrote:
>>
>> On 02/26/2013 05:06 PM, Stephen Warren wrote:
>>> On 02/26/2013 04:01 PM, Jon Hunter wrote:
>>>>
>>>> On 02/26/2013 04:44 PM, Stephen Warren wrote:
>>>>> On 02/26/2013 03:40 PM, Jon Hunter wrote:
>>>>>>
>>>>>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>>>>>>
>>>>>>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>>>>>>> GPMC for an OMAP3 SoC based board.
>>>>>>>
>>>>>>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>>>>>>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>>>>>>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>>>>>>
>>>>>>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>>>>>>> should be <2> and that a value of 8 is "active low level-sensitive".
>>>>>>>
>>>>>>> So I tried this:
>>>>>>>
>>>>>>> &gpmc {
>>>>>>> 	ethernet@5,0 {
>>>>>>> 		pinctrl-names = "default";
>>>>>>> 		pinctrl-0 = <&smsc911x_pins>;
>>>>>>> 		compatible = "smsc,lan9221", "smsc,lan9115";
>>>>>>> 		reg = <5 0 0xff>; /* CS5 */
>>>>>>> 		interrupt-parent = <&gpio6>;
>>>>>>> 		interrupts = <16 8>; /* gpio line 176 */
>>>>>>> 		interrupt-names = "smsc911x irq";
>>>>>>> 		vmmc-supply = <&vddvario>;
>>>>>>> 		vmmc_aux-supply = <&vdd33a>;
>>>>>>> 		reg-io-width = <4>;
>>>>>>>
>>>>>>> 		smsc,save-mac-address;
>>>>>>>       };
>>>>>>> };
>>>>>>
>>>>>> Are you requesting the gpio anywhere? If not then this is not going to
>>>>>> work as-is. This was discussed fairly recently [1] and the conclusion
>>>>>> was that the gpio needs to be requested before we can use as an interrupt.
>>>>>
>>>>> That seems wrong; the GPIO/IRQ driver should handle this internally. The
>>>>> Ethernet driver shouldn't know/care whether the interrupt it's given is
>>>>> some form of dedicated interrupt or a GPIO-based interrupt, and even if
>>>>> it somehow did, there's no irq_to_gpio() any more, so the driver can't
>>>>> tell which GPIO ID it should request, unless it's given yet another
>>>>> property to represent this.
>>>>
>>>> I agree that ideally this should be handled internally. Did you read the
>>>> discussion on the thread that I referenced [1]? If you have any thoughts
>>>> we are open to ideas :-)
>>>>
>>>> Cheers
>>>> Jon
>>>>
>>>> [1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
>>>
>>> Oh, when I clicked that link the first time, all I saw was the patch,
>>> not any discussion. I guess it must have timed out finding the other
>>> emails or something.
>>
>> Actually, I sent a slightly different link the 2nd time to ensure you
>> saw the thread. So my fault ;-)
>>
>>> I disagree that the GPIO needs to be requested, and that a custom DT
>>> property and associated code are needed for that; simply requesting the
>>> IRQ should be enough to make everything work.
>>>
>>> In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
>>> goes and configures the base GPIO HW to enable the pin as a GPIO, just
>>> like gpio_request() would. I imagine the OMAP driver can do whatever
>>> similar action it needs.
>>
>> Yes that is similar to what the patch in the thread was attempting to
>> do, but this got shot down.
>>
>> One issue I see is that by not calling gpio_request, then potentially
>> you could have someone request a gpio via gpio_request() and someone
>> trying to use it as an interrupt source via request_irq(). Now obviously
>> that represents a bug because there is only one physical gpio, but I
>> gather it is something we need to protect against.
> 
> I'm not sure it's really that much of an issue, but presumably the
> solution is for a combined GPIO+IRQ driver to simply call gpio_request
> internally from within some irq_chip function. It looks like struct
> irq_chip doesn't have a request/free, but perhaps they could be added to
> solve this?

Yes I was wondering if we could do something like that. That would work,
may be that's what we should propose.

Thanks
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Tue, Feb 26, 2013 at 11:40 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>
> [snip]
>
>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>
>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>> GPMC for an OMAP3 SoC based board.
>>
>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>
>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>> should be <2> and that a value of 8 is "active low level-sensitive".
>>
>> So I tried this:
>>
>> &gpmc {
>>       ethernet@5,0 {
>>               pinctrl-names = "default";
>>               pinctrl-0 = <&smsc911x_pins>;
>>               compatible = "smsc,lan9221", "smsc,lan9115";
>>               reg = <5 0 0xff>; /* CS5 */
>>               interrupt-parent = <&gpio6>;
>>               interrupts = <16 8>; /* gpio line 176 */
>>               interrupt-names = "smsc911x irq";
>>               vmmc-supply = <&vddvario>;
>>               vmmc_aux-supply = <&vdd33a>;
>>               reg-io-width = <4>;
>>
>>               smsc,save-mac-address;
>>       };
>> };
>
> Are you requesting the gpio anywhere? If not then this is not going to
> work as-is. This was discussed fairly recently [1] and the conclusion
> was that the gpio needs to be requested before we can use as an interrupt.
>
> I have not seen your latest smsc code for omap, but when you are
> requesting the gpmc chip-select you should also request the gpio.
>

Yes, I realized that requesting the gpio was necessary so what I did
is to use the "regulator-fixed" optional property "gpio" and define
the GPIO used as an IRQ in a regulator used by the SMSC chip. So, I
have this on my board DT:

vddvario: regulator-vddvario {
	compatible = "regulator-fixed";
	regulator-name = "vddvario";
	regulator-always-on;
	gpio = <&gpio6 16 8>;  /* gpio line 176 */
	enable-active-high;
	gpio-open-drain;
	regulator-boot-on;
};

&gpmc {
	ethernet@5,0 {
		pinctrl-names = "default";
		pinctrl-0 = <&smsc911x_pins>;
		compatible = "smsc,lan9221", "smsc,lan9115";
		reg = <5 0 0xff>;
		interrupt-parent = <&gpio6>;
		interrupts = <16 8>; /* gpio line 176 */
		interrupt-names = "smsc911x irq";
		vmmc-supply = <&vddvario>;
		vmmc_aux-supply = <&vdd33a>;
		reg-io-width = <4>;

		smsc,save-mac-address;
      };
};

That way a call to gpio_request_one() is made and the GPIO is requested.

This look a little hack-ish for me but I've seen this in other
DeviceTrees like omap4-sdp.dts so I thought it was a common DT
pattern.

>> But in the smsc911x probe function:
>>
>> irq_res->flags & IRQF_TRIGGER_MASK;
>>
>> returns 0 which means that no trigger flags where set.
>>
>> I took a look to the GPIOs device node definition on omap{3,4,5}.dtsi
>> and all look like this:
>>
>> e.g from omap3.dtsi:
>>
>>                 gpio6: gpio@49058000 {
>>                         compatible = "ti,omap3-gpio";
>>                         ti,hwmods = "gpio6";
>>                         gpio-controller;
>>                         #gpio-cells = <2>;
>>                         interrupt-controller;
>>                         #interrupt-cells = <1>;
>>                 };
>>
>> So, even when the documentation says that all the GPIO device nodes in
>> OMAP2+ should use a #interrupt-cells property value of <2>, they are
>> only using <1>. Changing that value to 2 makes hangs the kernel and it
>> does not boot.
>
> I will need to take a look at that. Is your code available anywhere so I
> can test?
>

Of course, I pushed a gpmc-smsc911x branch to my github linux repository [2].

The branch is latest Linus' master + Benoit's linux-omap-dt/for_3.9/dts +
"ARM: OMAP2+: Prevent potential crash if GPMC probe fails" [3] +
"ARM: dts: OMAP3: Add GPMC controller" [4] + my patches:

Javier Martinez Canillas (5):
      ARM: dts: OMAP3: reduce GPMC mapped registers address space
      ARM: dts: OMAP3: make GPMC node compatible with simple-bus
      ARM: dts: OMAP3: add ranges property for GPMC chip-select 5
      ARM: dts: omap3-igep0020: Add SMSC911x LAN chip support
      smsc: smc911x: (HACK) force "active low" polarity for IRQ

The last patch is just an ugly hack that forces the IRQ flags to
active low level-sensitive so the SMSC911x IRQ is triggered on my
board (IGEPv2).

I just added for testing purposes since the omap3-gpio
interrupt-controller #interrupt-cells = <2> seems to not be working
and I can't pass this flag when defining the IRQ in the smsc911x
ethernet device node.

> Cheers
> Jon
>

Thanks a lot for your help and best regards,
Javier

[1]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/92192
[2]: https://github.com/martinezjavier/linux.git
[3]: https://patchwork.kernel.org/patch/2118831/
[4]: https://patchwork.kernel.org/patch/2057111/

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Wed, Feb 27, 2013 at 12:08 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>
> [snip]
>
>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>
>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>> GPMC for an OMAP3 SoC based board.
>>
>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>
>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>> should be <2> and that a value of 8 is "active low level-sensitive".
>>
>> So I tried this:
>>
>> &gpmc {
>>       ethernet@5,0 {
>>               pinctrl-names = "default";
>>               pinctrl-0 = <&smsc911x_pins>;
>>               compatible = "smsc,lan9221", "smsc,lan9115";
>>               reg = <5 0 0xff>; /* CS5 */
>>               interrupt-parent = <&gpio6>;
>>               interrupts = <16 8>; /* gpio line 176 */
>>               interrupt-names = "smsc911x irq";
>>               vmmc-supply = <&vddvario>;
>>               vmmc_aux-supply = <&vdd33a>;
>>               reg-io-width = <4>;
>
> By the way, reg-io-width for omap does not look correct. The GPMC only
> supports 8-bit or 16-bit devices IIRC. I believe all my omap boards use
> 16-bit.
>

I thought that even when the GPMC was a 16-bit external memory
controller it could do some access adaptation to support 32-bit
devices.

By looking at the board files for others OMAP3 based boards
(board-{omap3evm,overo,zoom-debugboard}.c) most of them set the struct
omap_smsc911x_platform_data .flags member to SMSC911X_USE_32BIT.

And by looking at Documentation/devicetree/bindings/net/smsc911x.txt I
thought that the corresponding DT property for this flag was
"reg-io-width"

Anyway, I tried using both reg-io-width = <4> and reg-io-width = <2>.
The ethernet chip works with both of them and I don't see too much
difference in performance:

16-bit
round-trip min/avg/max = 0.611/0.738/0.946 ms

32-bit
round-trip min/avg/max = 0.519/0.690/0.976 ms

So, is your call ;-)

> Cheers
> Jon

Thanks a lot for your help,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Wed, Feb 27, 2013 at 2:07 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 02/26/2013 06:13 PM, Stephen Warren wrote:
>> On 02/26/2013 04:45 PM, Jon Hunter wrote:
>>>
>>> On 02/26/2013 05:06 PM, Stephen Warren wrote:
>>>> On 02/26/2013 04:01 PM, Jon Hunter wrote:
>>>>>
>>>>> On 02/26/2013 04:44 PM, Stephen Warren wrote:
>>>>>> On 02/26/2013 03:40 PM, Jon Hunter wrote:
>>>>>>>
>>>>>>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>>>>>>
>>>>>>> [snip]
>>>>>>>
>>>>>>>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>>>>>>>
>>>>>>>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>>>>>>>> GPMC for an OMAP3 SoC based board.
>>>>>>>>
>>>>>>>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>>>>>>>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>>>>>>>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>>>>>>>
>>>>>>>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>>>>>>>> should be <2> and that a value of 8 is "active low level-sensitive".
>>>>>>>>
>>>>>>>> So I tried this:
>>>>>>>>
>>>>>>>> &gpmc {
>>>>>>>>         ethernet@5,0 {
>>>>>>>>                 pinctrl-names = "default";
>>>>>>>>                 pinctrl-0 = <&smsc911x_pins>;
>>>>>>>>                 compatible = "smsc,lan9221", "smsc,lan9115";
>>>>>>>>                 reg = <5 0 0xff>; /* CS5 */
>>>>>>>>                 interrupt-parent = <&gpio6>;
>>>>>>>>                 interrupts = <16 8>; /* gpio line 176 */
>>>>>>>>                 interrupt-names = "smsc911x irq";
>>>>>>>>                 vmmc-supply = <&vddvario>;
>>>>>>>>                 vmmc_aux-supply = <&vdd33a>;
>>>>>>>>                 reg-io-width = <4>;
>>>>>>>>
>>>>>>>>                 smsc,save-mac-address;
>>>>>>>>       };
>>>>>>>> };
>>>>>>>
>>>>>>> Are you requesting the gpio anywhere? If not then this is not going to
>>>>>>> work as-is. This was discussed fairly recently [1] and the conclusion
>>>>>>> was that the gpio needs to be requested before we can use as an interrupt.
>>>>>>
>>>>>> That seems wrong; the GPIO/IRQ driver should handle this internally. The
>>>>>> Ethernet driver shouldn't know/care whether the interrupt it's given is
>>>>>> some form of dedicated interrupt or a GPIO-based interrupt, and even if
>>>>>> it somehow did, there's no irq_to_gpio() any more, so the driver can't
>>>>>> tell which GPIO ID it should request, unless it's given yet another
>>>>>> property to represent this.
>>>>>
>>>>> I agree that ideally this should be handled internally. Did you read the
>>>>> discussion on the thread that I referenced [1]? If you have any thoughts
>>>>> we are open to ideas :-)
>>>>>
>>>>> Cheers
>>>>> Jon
>>>>>
>>>>> [1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
>>>>
>>>> Oh, when I clicked that link the first time, all I saw was the patch,
>>>> not any discussion. I guess it must have timed out finding the other
>>>> emails or something.
>>>
>>> Actually, I sent a slightly different link the 2nd time to ensure you
>>> saw the thread. So my fault ;-)
>>>
>>>> I disagree that the GPIO needs to be requested, and that a custom DT
>>>> property and associated code are needed for that; simply requesting the
>>>> IRQ should be enough to make everything work.
>>>>
>>>> In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
>>>> goes and configures the base GPIO HW to enable the pin as a GPIO, just
>>>> like gpio_request() would. I imagine the OMAP driver can do whatever
>>>> similar action it needs.
>>>
>>> Yes that is similar to what the patch in the thread was attempting to
>>> do, but this got shot down.
>>>
>>> One issue I see is that by not calling gpio_request, then potentially
>>> you could have someone request a gpio via gpio_request() and someone
>>> trying to use it as an interrupt source via request_irq(). Now obviously
>>> that represents a bug because there is only one physical gpio, but I
>>> gather it is something we need to protect against.
>>
>> I'm not sure it's really that much of an issue, but presumably the
>> solution is for a combined GPIO+IRQ driver to simply call gpio_request
>> internally from within some irq_chip function. It looks like struct
>> irq_chip doesn't have a request/free, but perhaps they could be added to
>> solve this?
>
> Yes I was wondering if we could do something like that. That would work,
> may be that's what we should propose.
>
> Thanks
> Jon

Something like that would definitely solve the GPIO request issue but
we still have the issue that the current OMAP GPIO controller binding
does not support #interrupt-cells = <2>.

So, we can't pass the trigger type and level flags for an IRQ-GPIO
when using an GPIO controller as the interrupt-parent for a device
node.

Do you have any comments on that issue?

I'll try to check Stephen's pointers but I'm not familiar with the
gpio-omap driver neither gpiolib.

Best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 02/26/2013 08:33 PM, Javier Martinez Canillas wrote:
...
> Yes, I realized that requesting the gpio was necessary so what I did
> is to use the "regulator-fixed" optional property "gpio" and define
> the GPIO used as an IRQ in a regulator used by the SMSC chip. So, I
> have this on my board DT:
> 
> vddvario: regulator-vddvario {
> 	compatible = "regulator-fixed";
> 	regulator-name = "vddvario";
> 	regulator-always-on;
> 	gpio = <&gpio6 16 8>;  /* gpio line 176 */
> 	enable-active-high;
> 	gpio-open-drain;

While admittedly it's configured as open-drain, that will configure the
GPIO to be an output, whereas for usage as an interrupt, it really
should be configured as an input... Perhaps it makes no difference on
OMAP HW since the I/O paths are separate, but I can easily imagine HW
where this hack wouldn't work.

> 	regulator-boot-on;
> };
> 
> &gpmc {
> 	ethernet@5,0 {
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&smsc911x_pins>;
> 		compatible = "smsc,lan9221", "smsc,lan9115";
> 		reg = <5 0 0xff>;
> 		interrupt-parent = <&gpio6>;
> 		interrupts = <16 8>; /* gpio line 176 */
> 		interrupt-names = "smsc911x irq";
> 		vmmc-supply = <&vddvario>;
> 		vmmc_aux-supply = <&vdd33a>;
> 		reg-io-width = <4>;
> 
> 		smsc,save-mac-address;
>       };
> };
> 
> That way a call to gpio_request_one() is made and the GPIO is requested.
> 
> This look a little hack-ish for me but I've seen this in other
> DeviceTrees like omap4-sdp.dts so I thought it was a common DT
> pattern.

Indeed; the GPIO is in no way a regulator, so while the above is fine
for testing, it's in no way a solution that can actually be used and
checked in.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 02/26/2013 08:57 PM, Javier Martinez Canillas wrote:
> On Wed, Feb 27, 2013 at 2:07 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>>
>> On 02/26/2013 06:13 PM, Stephen Warren wrote:
>>> On 02/26/2013 04:45 PM, Jon Hunter wrote:
...
>>>> One issue I see is that by not calling gpio_request, then potentially
>>>> you could have someone request a gpio via gpio_request() and someone
>>>> trying to use it as an interrupt source via request_irq(). Now obviously
>>>> that represents a bug because there is only one physical gpio, but I
>>>> gather it is something we need to protect against.
>>>
>>> I'm not sure it's really that much of an issue, but presumably the
>>> solution is for a combined GPIO+IRQ driver to simply call gpio_request
>>> internally from within some irq_chip function. It looks like struct
>>> irq_chip doesn't have a request/free, but perhaps they could be added to
>>> solve this?
>>
>> Yes I was wondering if we could do something like that. That would work,
>> may be that's what we should propose.
> 
> Something like that would definitely solve the GPIO request issue but
> we still have the issue that the current OMAP GPIO controller binding
> does not support #interrupt-cells = <2>.

The binding documentation in
Documentation/devicetree/bindings/gpio/gpio-omap.txt indicates that it
does. If this doesn't work in practice, it's a driver bug that can
presumably be easily fixed. And no need to change any ABI definitions:-)

BTW, I notice in that binding document that the description of the two
cells for #interrupt-cells is actually part of the description of the
"interrupt-controller" property; it should be moved up one line really.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Wed, Feb 27, 2013 at 6:47 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/26/2013 08:33 PM, Javier Martinez Canillas wrote:
> ...
>> Yes, I realized that requesting the gpio was necessary so what I did
>> is to use the "regulator-fixed" optional property "gpio" and define
>> the GPIO used as an IRQ in a regulator used by the SMSC chip. So, I
>> have this on my board DT:
>>
>> vddvario: regulator-vddvario {
>>       compatible = "regulator-fixed";
>>       regulator-name = "vddvario";
>>       regulator-always-on;
>>       gpio = <&gpio6 16 8>;  /* gpio line 176 */
>>       enable-active-high;
>>       gpio-open-drain;
>
> While admittedly it's configured as open-drain, that will configure the
> GPIO to be an output, whereas for usage as an interrupt, it really
> should be configured as an input... Perhaps it makes no difference on
> OMAP HW since the I/O paths are separate, but I can easily imagine HW
> where this hack wouldn't work.
>
>>       regulator-boot-on;
>> };
>>
>> &gpmc {
>>       ethernet@5,0 {
>>               pinctrl-names = "default";
>>               pinctrl-0 = <&smsc911x_pins>;
>>               compatible = "smsc,lan9221", "smsc,lan9115";
>>               reg = <5 0 0xff>;
>>               interrupt-parent = <&gpio6>;
>>               interrupts = <16 8>; /* gpio line 176 */
>>               interrupt-names = "smsc911x irq";
>>               vmmc-supply = <&vddvario>;
>>               vmmc_aux-supply = <&vdd33a>;
>>               reg-io-width = <4>;
>>
>>               smsc,save-mac-address;
>>       };
>> };
>>
>> That way a call to gpio_request_one() is made and the GPIO is requested.
>>
>> This look a little hack-ish for me but I've seen this in other
>> DeviceTrees like omap4-sdp.dts so I thought it was a common DT
>> pattern.
>
> Indeed; the GPIO is in no way a regulator, so while the above is fine
> for testing, it's in no way a solution that can actually be used and
> checked in.

Yes, this is just a work-in-progress and is not meant to be taken as a
patch submission.

I'm just doing some testing with DT and figuring out how much effort
will take to have the same hardware support we currently with board
files for IGEP boards.

Thanks a lot for your feedback and best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Wed, Feb 27, 2013 at 6:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/26/2013 08:57 PM, Javier Martinez Canillas wrote:
>> On Wed, Feb 27, 2013 at 2:07 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>>>
>>> On 02/26/2013 06:13 PM, Stephen Warren wrote:
>>>> On 02/26/2013 04:45 PM, Jon Hunter wrote:
> ...
>>>>> One issue I see is that by not calling gpio_request, then potentially
>>>>> you could have someone request a gpio via gpio_request() and someone
>>>>> trying to use it as an interrupt source via request_irq(). Now obviously
>>>>> that represents a bug because there is only one physical gpio, but I
>>>>> gather it is something we need to protect against.
>>>>
>>>> I'm not sure it's really that much of an issue, but presumably the
>>>> solution is for a combined GPIO+IRQ driver to simply call gpio_request
>>>> internally from within some irq_chip function. It looks like struct
>>>> irq_chip doesn't have a request/free, but perhaps they could be added to
>>>> solve this?
>>>
>>> Yes I was wondering if we could do something like that. That would work,
>>> may be that's what we should propose.
>>
>> Something like that would definitely solve the GPIO request issue but
>> we still have the issue that the current OMAP GPIO controller binding
>> does not support #interrupt-cells = <2>.
>
> The binding documentation in
> Documentation/devicetree/bindings/gpio/gpio-omap.txt indicates that it
> does. If this doesn't work in practice, it's a driver bug that can
> presumably be easily fixed. And no need to change any ABI definitions:-)
>

indeed :-)

In fact I think that some documentation bits were borrowed from the
NVIDIA Tegra GPIO controller bindings but it was never implemented in
the OMAP GPIO driver to parse the second interrupt-cell which should
specify the flags.

> BTW, I notice in that binding document that the description of the two
> cells for #interrupt-cells is actually part of the description of the
> "interrupt-controller" property; it should be moved up one line really.

Right, will submit a patch to fix this.

Best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 02/26/2013 09:47 PM, Javier Martinez Canillas wrote:
> On Wed, Feb 27, 2013 at 12:08 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>>
>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>
>> [snip]
>>
>>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>>
>>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>>> GPMC for an OMAP3 SoC based board.
>>>
>>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>>
>>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>>> should be <2> and that a value of 8 is "active low level-sensitive".
>>>
>>> So I tried this:
>>>
>>> &gpmc {
>>>       ethernet@5,0 {
>>>               pinctrl-names = "default";
>>>               pinctrl-0 = <&smsc911x_pins>;
>>>               compatible = "smsc,lan9221", "smsc,lan9115";
>>>               reg = <5 0 0xff>; /* CS5 */
>>>               interrupt-parent = <&gpio6>;
>>>               interrupts = <16 8>; /* gpio line 176 */
>>>               interrupt-names = "smsc911x irq";
>>>               vmmc-supply = <&vddvario>;
>>>               vmmc_aux-supply = <&vdd33a>;
>>>               reg-io-width = <4>;
>>
>> By the way, reg-io-width for omap does not look correct. The GPMC only
>> supports 8-bit or 16-bit devices IIRC. I believe all my omap boards use
>> 16-bit.
>>
> 
> I thought that even when the GPMC was a 16-bit external memory
> controller it could do some access adaptation to support 32-bit
> devices.

Right the GPMC itself may be able to translate 32-bit accesses into
16-bit. However, I have not looked at the data sheet for the smsc parts
to know what impact this would have on any register accesses in the smsc
device.

> By looking at the board files for others OMAP3 based boards
> (board-{omap3evm,overo,zoom-debugboard}.c) most of them set the struct
> omap_smsc911x_platform_data .flags member to SMSC911X_USE_32BIT.

I see this too now. I was looking at the
arch/arm/mach-omap2/gpmc-smc91x.c which has SMC91X_USE_16BIT and
GPMC_CONFIG1_DEVICESIZE_16. So this is for a slightly different ethernet
chip but same concept. Hopefully, someone knew what they were doing for
those other boards!

> And by looking at Documentation/devicetree/bindings/net/smsc911x.txt I
> thought that the corresponding DT property for this flag was
> "reg-io-width"
> 
> Anyway, I tried using both reg-io-width = <4> and reg-io-width = <2>.
> The ethernet chip works with both of them and I don't see too much
> difference in performance:
> 
> 16-bit
> round-trip min/avg/max = 0.611/0.738/0.946 ms
> 
> 32-bit
> round-trip min/avg/max = 0.519/0.690/0.976 ms
> 
> So, is your call ;-)

While I can't argue with the fact it works, I would not be prepared to
make that call without reviewing the documentation for the smsc and the
driver.

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 02/26/2013 09:57 PM, Javier Martinez Canillas wrote:

[snip]

> Something like that would definitely solve the GPIO request issue but
> we still have the issue that the current OMAP GPIO controller binding
> does not support #interrupt-cells = <2>.
> 
> So, we can't pass the trigger type and level flags for an IRQ-GPIO
> when using an GPIO controller as the interrupt-parent for a device
> node.
> 
> Do you have any comments on that issue?

Can you elaborate a bit more on why you say this is not supported?

I have been playing with this today on an omap board and if I set the
#interrupt-cells = <2>, then I do see that irq_domain_xlate_onetwocell()
is called and the irq number and flags read as expected. Following which
I then see it will call the omap_irq_type() to set type. So AFAICT it works.

Please note I do see that when the SMC driver calls request_irq() in
smc_drv_probe() it is also settings the trigger type to
IRQ_TYPE_EDGE_RISING (default). So if you are setting to low-level
sensitive in DT, then this is being overwritten. We could fix this by
setting SMC_IRQ_FLAGS to -1 for OMAP.

In general we do need to fix the gpio binding for omap to default to
#interrupt-cell = <2>, as this should work. However, before we can do
that we need to fix the issue of ensuring the gpio module is enabled if
being used as an interrupt source without having to call gpio_request()
first.

We should probably add the following patch as well to avoid any hangs if
the bank is not enabled, when omap_irq_type is called.

commit 5e298de564e09f5ca4148a9bc0ed5d16b4742f14
Author: Jon Hunter <jon-hunter@ti.com>
Date:   Wed Feb 27 17:14:11 2013 -0600

    gpio/omap: warn if gpio bank is not enabled on setting irq type

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index f1fbedb2..cbdc796 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -421,6 +421,9 @@ static int gpio_irq_type(struct irq_data *d,
unsigned type)
        int retval;
        unsigned long flags;

+       if (WARN_ON(!bank->mod_usage))
+               return -EINVAL;
+
 #ifdef CONFIG_ARCH_OMAP1
        if (d->irq > IH_MPUIO_BASE)
                gpio = OMAP_MPUIO(d->irq - IH_MPUIO_BASE);


Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson <b-cousson@ti.com> wrote:

> @@ -52,7 +55,8 @@ struct gpio_bank {
>         struct list_head node;
>         void __iomem *base;
>         u16 irq;
> -       u16 virtual_irq_start;
> +       int irq_base;
> +       struct irq_domain *domain;

This seems wrong. IRQ domains are used to avoid keeping track of
irq base offsets. I would even say it's the whole point of irq domains.

> @@ -669,7 +673,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>                 if (!isr)
>                         break;
>
> -               gpio_irq = bank->virtual_irq_start;
> +               gpio_irq = bank->irq_base;
>                 for (; isr != 0; isr >>= 1, gpio_irq++) {
>                         gpio_index = GPIO_INDEX(bank, irq_to_gpio(gpio_irq));
>

Use irq_find_mapping(irqdomain, hwirq) in this function.


> @@ -915,7 +919,7 @@ static int gpio_2irq(struct gpio_chip *chip, unsigned offset)
>         struct gpio_bank *bank;
>
>         bank = container_of(chip, struct gpio_bank, chip);
> -       return bank->virtual_irq_start + offset;
> +       return bank->irq_base + offset;

Use irq_create_mapping() in this function.

> +       bank->irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
> +       if (bank->irq_base < 0) {
> +               dev_err(dev, "Couldn't allocate IRQ numbers\n");
> +               return -ENODEV;
> +       }
> +
> +       bank->domain = irq_domain_add_legacy(node, bank->width, bank->irq_base,
> +                                            0, &irq_domain_simple_ops, NULL);

Use irq_domain_add_simple() and the descs will be allocated for
you as part of the domain creation, when using a pre-fixed base.

If you fix this I suspect the device tree discussion also will fix
itself :-)

Documentation/IRQ-domain.txt

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Thu, Feb 28, 2013 at 12:16 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 02/26/2013 09:57 PM, Javier Martinez Canillas wrote:
>
> [snip]
>
>> Something like that would definitely solve the GPIO request issue but
>> we still have the issue that the current OMAP GPIO controller binding
>> does not support #interrupt-cells = <2>.
>>
>> So, we can't pass the trigger type and level flags for an IRQ-GPIO
>> when using an GPIO controller as the interrupt-parent for a device
>> node.
>>
>> Do you have any comments on that issue?
>
> Can you elaborate a bit more on why you say this is not supported?
>
> I have been playing with this today on an omap board and if I set the
> #interrupt-cells = <2>, then I do see that irq_domain_xlate_onetwocell()
> is called and the irq number and flags read as expected. Following which
> I then see it will call the omap_irq_type() to set type. So AFAICT it works.
>

Yes, it does.

I (wrongly) assumed that it was not working since the GPIO OMAP
binding documentation says that #interrupt-cells should be <2> but all
OMAP2+ DTs use <1> instead. Also, when trying to change to <2> I had
the kernel hang.

But it was indeed that the GPIO bank was not enabled before calling
gpio_irq_type() and this made the kernel to hang. Your patch fixed the
issue and allowed me to find the cause.

The problem was that when using the DT hack of defining the GPIO in
the ethernet chip regulator,  the calls to
irq_domain_xlate_onetwocell() and gpio_irq_type() were made before the
call to gpio_request_one() so the GPIO bank was not enabled.

If gpio_request() is called in gpmc_probe_dt(), then the GPIO bank is
enabled and gpio_irq_type() succeeds.

So, it was just me being stupid and don't understanding the implementation.

> Please note I do see that when the SMC driver calls request_irq() in
> smc_drv_probe() it is also settings the trigger type to
> IRQ_TYPE_EDGE_RISING (default). So if you are setting to low-level
> sensitive in DT, then this is being overwritten. We could fix this by
> setting SMC_IRQ_FLAGS to -1 for OMAP.
>

Please note that I'm using a SMSC 911x chip and not a SMSC 91x, so the
driver is not smc91x but smc911x. It has the same behaviour though
(IRQ flags overwritten somehow), just to be sure that we are on the
same page.

I don't know if just setting SMC_IRQ_FLAGS to -1 will be enough to fix
the smc91x since request_irq() is just passing whatever value is in
irq_flags.

By looking at the two drivers (smc91x and smsc911x) it seems that the
only difference on how they manage the flags is that smc91x does:

unsigned long irq_flags = SMC_IRQ_FLAGS;
...
       if (irq_flags == -1 || ires->flags & IRQF_TRIGGER_MASK)
                irq_flags = ires->flags & IRQF_TRIGGER_MASK;

while smsc911x driver's probe function uses the flags from the
resource unconditionally:

irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;

So, at the end both will set irq_flags to whatever is on the
IORESOURCE_IRQ struct resource flags member.

The real problem is irq_flags to be 0 instead of the value defined on
the second cell of the "interrupts" property.

when irq_domain_xlate_onetwocell() is called for the ethernet GPIO-IRQ
I see that both the cells size and the second cell with the flag
values are set correctly (2 and IRQF_TRIGGER_LOW).

But even when gpio_irq_type() succeeds it seems that the struct
resource IRQ flags passed to the smsc911x driver is still overwritten
or not set correctly since flags & IRQF_TRIGGER_MASK is always 0.

> In general we do need to fix the gpio binding for omap to default to
> #interrupt-cell = <2>, as this should work. However, before we can do
> that we need to fix the issue of ensuring the gpio module is enabled if
> being used as an interrupt source without having to call gpio_request()
> first.
>

Indeed, although I still wonder why the flags are not set correctly
for the smsc911x driver.

I had only tested it with this device so I don't know if this is a
general issue of IORESOURCE_IRQ structs not been initialized correctly
when using GPIOs as IRQ on OMAP or if is only related to this driver.

> We should probably add the following patch as well to avoid any hangs if
> the bank is not enabled, when omap_irq_type is called.
>
> commit 5e298de564e09f5ca4148a9bc0ed5d16b4742f14
> Author: Jon Hunter <jon-hunter@ti.com>
> Date:   Wed Feb 27 17:14:11 2013 -0600
>
>     gpio/omap: warn if gpio bank is not enabled on setting irq type
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index f1fbedb2..cbdc796 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -421,6 +421,9 @@ static int gpio_irq_type(struct irq_data *d,
> unsigned type)
>         int retval;
>         unsigned long flags;
>
> +       if (WARN_ON(!bank->mod_usage))
> +               return -EINVAL;
> +
>  #ifdef CONFIG_ARCH_OMAP1
>         if (d->irq > IH_MPUIO_BASE)
>                 gpio = OMAP_MPUIO(d->irq - IH_MPUIO_BASE);
>
>
> Cheers
> Jon
>
>

Thanks a lot for your help and best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Benoit Cousson]

+ Jon who was brave enough to take over the OMAP GPIO  driver
+ New email address for Kevin since he is no longer at TI :-(.
- Tarun that left TI but I don't have his new email

Hi Linus,

On 02/28/2013 12:41 AM, Linus Walleij wrote:
> On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson <b-cousson@ti.com> wrote:

Gosh! That's pretty old stuff :-)

>> @@ -52,7 +55,8 @@ struct gpio_bank {
>>         struct list_head node;
>>         void __iomem *base;
>>         u16 irq;
>> -       u16 virtual_irq_start;
>> +       int irq_base;
>> +       struct irq_domain *domain;
> 
> This seems wrong. IRQ domains are used to avoid keeping track of
> irq base offsets. I would even say it's the whole point of irq domains.

Well at that time, it was not the only point. We were trying to boot
with DT, and for that irq_domain was mandatory. At the very same time,
Grant was cleaning and consolidating the whole irq_domain
infrastructure, and thus most of the following API were not really
stabilized or even available.

So, this idea was to avoid any regression while allowing DT boot... And
obviously a lot of stuff happened since that good old time.

>> @@ -669,7 +673,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>>                 if (!isr)
>>                         break;
>>
>> -               gpio_irq = bank->virtual_irq_start;
>> +               gpio_irqkhilman@ti.com = bank->irq_base;
>>                 for (; isr != 0; isr >>= 1, gpio_irq++) {
>>                         gpio_index = GPIO_INDEX(bank, irq_to_gpio(gpio_irq));
>>
> 
> Use irq_find_mapping(irqdomain, hwirq) in this function.
> 
> 
>> @@ -915,7 +919,7 @@ static int gpio_2irq(struct gpio_chip *chip, unsigned offset)
>>         struct gpio_bank *bank;
>>
>>         bank = container_of(chip, struct gpio_bank, chip);
>> -       return bank->virtual_irq_start + offset;
>> +       return bank->irq_base + offset;
> 
> Use irq_create_mapping() in this function.
> 
>> +       bank->irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
>> +       if (bank->irq_base < 0) {
>> +               dev_err(dev, "Couldn't allocate IRQ numbers\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       bank->domain = irq_domain_add_legacy(node, bank->width, bank->irq_base,
>> +                                            0, &irq_domain_simple_ops, NULL);
> 
> Use irq_domain_add_simple() and the descs will be allocated for
> you as part of the domain creation, when using a pre-fixed base.

Funny, that API was removed while I was writing this patch, and is now
back in town...

> If you fix this I suspect the device tree discussion also will fix
> itself :-)

Mmm, I hope it is the case, but I'm not sure it will be that easy :-)

Thanks,
Benoit

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 02/28/2013 06:17 AM, Javier Martinez Canillas wrote:
> On Thu, Feb 28, 2013 at 12:16 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>>
>> On 02/26/2013 09:57 PM, Javier Martinez Canillas wrote:
>>
>> [snip]
>>
>>> Something like that would definitely solve the GPIO request issue but
>>> we still have the issue that the current OMAP GPIO controller binding
>>> does not support #interrupt-cells = <2>.
>>>
>>> So, we can't pass the trigger type and level flags for an IRQ-GPIO
>>> when using an GPIO controller as the interrupt-parent for a device
>>> node.
>>>
>>> Do you have any comments on that issue?
>>
>> Can you elaborate a bit more on why you say this is not supported?
>>
>> I have been playing with this today on an omap board and if I set the
>> #interrupt-cells = <2>, then I do see that irq_domain_xlate_onetwocell()
>> is called and the irq number and flags read as expected. Following which
>> I then see it will call the omap_irq_type() to set type. So AFAICT it works.
>>
> 
> Yes, it does.
> 
> I (wrongly) assumed that it was not working since the GPIO OMAP
> binding documentation says that #interrupt-cells should be <2> but all
> OMAP2+ DTs use <1> instead. Also, when trying to change to <2> I had
> the kernel hang.
> 
> But it was indeed that the GPIO bank was not enabled before calling
> gpio_irq_type() and this made the kernel to hang. Your patch fixed the
> issue and allowed me to find the cause.
> 
> The problem was that when using the DT hack of defining the GPIO in
> the ethernet chip regulator,  the calls to
> irq_domain_xlate_onetwocell() and gpio_irq_type() were made before the
> call to gpio_request_one() so the GPIO bank was not enabled.
> 
> If gpio_request() is called in gpmc_probe_dt(), then the GPIO bank is
> enabled and gpio_irq_type() succeeds.
> 
> So, it was just me being stupid and don't understanding the implementation.

No problem. Glad we are on the same page :-)

>> Please note I do see that when the SMC driver calls request_irq() in
>> smc_drv_probe() it is also settings the trigger type to
>> IRQ_TYPE_EDGE_RISING (default). So if you are setting to low-level
>> sensitive in DT, then this is being overwritten. We could fix this by
>> setting SMC_IRQ_FLAGS to -1 for OMAP.
>>
> 
> Please note that I'm using a SMSC 911x chip and not a SMSC 91x, so the
> driver is not smc91x but smc911x. It has the same behaviour though
> (IRQ flags overwritten somehow), just to be sure that we are on the
> same page.
> 
> I don't know if just setting SMC_IRQ_FLAGS to -1 will be enough to fix
> the smc91x since request_irq() is just passing whatever value is in
> irq_flags.
> 
> By looking at the two drivers (smc91x and smsc911x) it seems that the
> only difference on how they manage the flags is that smc91x does:
> 
> unsigned long irq_flags = SMC_IRQ_FLAGS;
> ...
>        if (irq_flags == -1 || ires->flags & IRQF_TRIGGER_MASK)
>                 irq_flags = ires->flags & IRQF_TRIGGER_MASK;
> 
> while smsc911x driver's probe function uses the flags from the
> resource unconditionally:
> 
> irq_flags = irq_res->flags & IRQF_TRIGGER_MASK;
> 
> So, at the end both will set irq_flags to whatever is on the
> IORESOURCE_IRQ struct resource flags member.

Actually, that's not the case for smc91x. By default SMC_IRQ_FLAGS != -1
(for omap) and so it will not set irq_flags to ires->flags &
IRQF_TRIGGER_MASK. However, if I force irq_flags to be -1, then I see
that irq_flags are to 0.

> The real problem is irq_flags to be 0 instead of the value defined on
> the second cell of the "interrupts" property.

So the resource flags for each irq is set in
of_irq_to_resource() which just does ...

	r->start = r->end = irq;
	r->flags = IORESOURCE_IRQ;
	r->name = name ? name : dev->full_name;


of_irq_to_resource() calls irq_to_parse_and_map() which eventually just
calls irq_set_irq_type() but type/flags is not returned and not populated.

I am wondering if this is intentional. The irq_type is being correctly
configured by irq_set_irq_type() when of_irq_to_resource() is called. In
the smc driver, if irq_flags are 0, then when request_irq() is called
the trigger type will not be set again (which is ok). __setup_irq has
the following ...

	/* Setup the type (level, edge polarity) if configured: */
	if (new->flags & IRQF_TRIGGER_MASK) {
		ret = __irq_set_trigger(desc, irq,
			new->flags & IRQF_TRIGGER_MASK);

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Thu, Feb 28, 2013 at 2:04 PM, Benoit Cousson <b-cousson@ti.com> wrote:

> On 02/28/2013 12:41 AM, Linus Walleij wrote:
>> On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson <b-cousson@ti.com> wrote:
>
> Gosh! That's pretty old stuff :-)

Hm yeah, haha :-)

I didn't notice that someone was replying in the thread for
some other reason (or maybe using the same subject and
confusing my gmail reader?).

Anyway, the basic idea is sound, so are you still working
on the patch? I remember discussing this with someone
in Denmark...

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 02/28/2013 06:09 PM, Linus Walleij wrote:
> On Thu, Feb 28, 2013 at 2:04 PM, Benoit Cousson <b-cousson@ti.com> wrote:
> 
>> On 02/28/2013 12:41 AM, Linus Walleij wrote:
>>> On Wed, Feb 15, 2012 at 5:04 PM, Benoit Cousson <b-cousson@ti.com> wrote:
>>
>> Gosh! That's pretty old stuff :-)
> 
> Hm yeah, haha :-)
> 
> I didn't notice that someone was replying in the thread for
> some other reason (or maybe using the same subject and
> confusing my gmail reader?).
> 
> Anyway, the basic idea is sound, so are you still working
> on the patch? I remember discussing this with someone
> in Denmark...

That was me :-)

I have the patch, I am testing and will send tomorrow for review.

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Grant Likely]

On Tue, 26 Feb 2013 17:01:22 -0600, Jon Hunter <jon-hunter-l0cyMroinI0@public.gmane.org> wrote:
> 
> On 02/26/2013 04:44 PM, Stephen Warren wrote:
> > On 02/26/2013 03:40 PM, Jon Hunter wrote:
> >> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
> >> Are you requesting the gpio anywhere? If not then this is not going to
> >> work as-is. This was discussed fairly recently [1] and the conclusion
> >> was that the gpio needs to be requested before we can use as an interrupt.
> > 
> > That seems wrong; the GPIO/IRQ driver should handle this internally. The
> > Ethernet driver shouldn't know/care whether the interrupt it's given is
> > some form of dedicated interrupt or a GPIO-based interrupt, and even if
> > it somehow did, there's no irq_to_gpio() any more, so the driver can't
> > tell which GPIO ID it should request, unless it's given yet another
> > property to represent this.
> 
> I agree that ideally this should be handled internally. Did you read the
> discussion on the thread that I referenced [1]? If you have any thoughts
> we are open to ideas :-)

I'm on an airplane right now, but I agree 100% with Stephen. I'll try to
remember to go read that thread and respond, but this falls firmly in
the its-a-bug category for me.  :-)

g.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 03/02/2013 02:05 PM, Grant Likely wrote:
> On Tue, 26 Feb 2013 17:01:22 -0600, Jon Hunter <jon-hunter@ti.com> wrote:
>>
>> On 02/26/2013 04:44 PM, Stephen Warren wrote:
>>> On 02/26/2013 03:40 PM, Jon Hunter wrote:
>>>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>>> Are you requesting the gpio anywhere? If not then this is not going to
>>>> work as-is. This was discussed fairly recently [1] and the conclusion
>>>> was that the gpio needs to be requested before we can use as an interrupt.
>>>
>>> That seems wrong; the GPIO/IRQ driver should handle this internally. The
>>> Ethernet driver shouldn't know/care whether the interrupt it's given is
>>> some form of dedicated interrupt or a GPIO-based interrupt, and even if
>>> it somehow did, there's no irq_to_gpio() any more, so the driver can't
>>> tell which GPIO ID it should request, unless it's given yet another
>>> property to represent this.
>>
>> I agree that ideally this should be handled internally. Did you read the
>> discussion on the thread that I referenced [1]? If you have any thoughts
>> we are open to ideas :-)
> 
> I'm on an airplane right now, but I agree 100% with Stephen. I'll try to
> remember to go read that thread and respond, but this falls firmly in
> the its-a-bug category for me.  :-)

Grant, did you have chance to review the thread [1]?

I am trying to figure out if we should just take the original patch
proposed in the thread (although Linus had some objections) or look at
alternative solutions such as adding a irq_chip request as Stephen
suggested.

Cheers
Jon

[1] comments.gmane.org/gmane.linux.ports.arm.omap/92192

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Fri, Mar 8, 2013 at 12:14 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 03/02/2013 02:05 PM, Grant Likely wrote:
>> On Tue, 26 Feb 2013 17:01:22 -0600, Jon Hunter <jon-hunter@ti.com> wrote:
>>>
>>> On 02/26/2013 04:44 PM, Stephen Warren wrote:
>>>> On 02/26/2013 03:40 PM, Jon Hunter wrote:
>>>>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>>>> Are you requesting the gpio anywhere? If not then this is not going to
>>>>> work as-is. This was discussed fairly recently [1] and the conclusion
>>>>> was that the gpio needs to be requested before we can use as an interrupt.
>>>>
>>>> That seems wrong; the GPIO/IRQ driver should handle this internally. The
>>>> Ethernet driver shouldn't know/care whether the interrupt it's given is
>>>> some form of dedicated interrupt or a GPIO-based interrupt, and even if
>>>> it somehow did, there's no irq_to_gpio() any more, so the driver can't
>>>> tell which GPIO ID it should request, unless it's given yet another
>>>> property to represent this.
>>>
>>> I agree that ideally this should be handled internally. Did you read the
>>> discussion on the thread that I referenced [1]? If you have any thoughts
>>> we are open to ideas :-)
>>
>> I'm on an airplane right now, but I agree 100% with Stephen. I'll try to
>> remember to go read that thread and respond, but this falls firmly in
>> the its-a-bug category for me.  :-)
>
> Grant, did you have chance to review the thread [1]?
>
> I am trying to figure out if we should just take the original patch
> proposed in the thread (although Linus had some objections) or look at
> alternative solutions such as adding a irq_chip request as Stephen
> suggested.
>
> Cheers
> Jon
>
> [1] comments.gmane.org/gmane.linux.ports.arm.omap/92192

Hello Grant,

I was wondering if you have any opinions on this issue. As Jon said,
Stephen proposed [2] to add a request callback to irq_chip.

I hacked a very simple and naive patch (just to validate the idea) and
is working. The GPIO bank is requested before calling the gpio-omap
.irq_set_type function handler (gpio_irq_type) when using a GPIO as an
IRQ on a DT. So is not necessary to call it explicitly anymore.

But the patch is obviously wrong (to say the least) since the kernel
runtime locking validator complains that "possible circular locking
dependency detected"

I just wanted to know if I was on the right track or completely lost here.

Thanks a lot and best regards,
javier

[2]: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85592.html

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..f5feb43 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -807,6 +807,13 @@ static void gpio_unmask_irq(struct irq_data *d)
 	spin_unlock_irqrestore(&bank->lock, flags);
 }

+static int gpio_irq_request(struct irq_data *d)
+{
+	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+	return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
+}
+
 static struct irq_chip gpio_irq_chip = {
 	.name		= "GPIO",
 	.irq_shutdown	= gpio_irq_shutdown,
@@ -815,6 +822,7 @@ static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.irq_request    = gpio_irq_request,
 };

 /*---------------------------------------------------------------------*/
diff --git a/include/linux/irq.h b/include/linux/irq.h
index bc4e066..2aeaa24 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -303,6 +303,7 @@ struct irq_chip {
 	void		(*irq_shutdown)(struct irq_data *data);
 	void		(*irq_enable)(struct irq_data *data);
 	void		(*irq_disable)(struct irq_data *data);
+	int             (*irq_request)(struct irq_data *data);

 	void		(*irq_ack)(struct irq_data *data);
 	void		(*irq_mask)(struct irq_data *data);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..07c20f7 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1093,6 +1093,13 @@ __setup_irq(unsigned int irq, struct irq_desc
*desc, struct irqaction *new)
 	if (!shared) {
 		init_waitqueue_head(&desc->wait_for_threads);

+		if (desc->irq_data.chip->irq_request) {
+			ret = desc->irq_data.chip->irq_request(&desc->irq_data);
+
+			if (ret)
+				goto out_mask;
+		}
+
 		/* Setup the type (level, edge polarity) if configured: */
 		if (new->flags & IRQF_TRIGGER_MASK) {
 			ret = __irq_set_trigger(desc, irq,

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Fri, Mar 15, 2013 at 12:21 PM, Javier Martinez Canillas
<martinez.javier@gmail.com> wrote:

> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 159f5c5..f5feb43 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -807,6 +807,13 @@ static void gpio_unmask_irq(struct irq_data *d)
>         spin_unlock_irqrestore(&bank->lock, flags);
>  }
>
> +static int gpio_irq_request(struct irq_data *d)
> +{
> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +
> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
> +}
> +
>  static struct irq_chip gpio_irq_chip = {
>         .name           = "GPIO",
>         .irq_shutdown   = gpio_irq_shutdown,
> @@ -815,6 +822,7 @@ static struct irq_chip gpio_irq_chip = {
>         .irq_unmask     = gpio_unmask_irq,
>         .irq_set_type   = gpio_irq_type,
>         .irq_set_wake   = gpio_wake_enable,
> +       .irq_request    = gpio_irq_request,
>  };

This is just turning everything on it's head. The normal order of things
is this sequence for GPIO 14 something like:

gpio_request(14);
int irq = gpio_to_irq(14);
request_any_context_irq(irq);

I understand that the approach take make things easier for device tree
but it screws up the semantics of the entire kernel (the lockdep etc
warnings are just a symptom), we are locked to the above semantic
sequence: you know the GPIO, *then* you get the IRQ, not the
other way around.

irq_to_gpio has been deemed a legacy thing that we want to get
rid of. The reason being that it is ambiguous on most every GPIO
expander out there. It is not supported by GPIOLIB. This is the
reason to why the implementation in the GPIOLIB in
<linux/gpio.h> looks like this:

static inline int irq_to_gpio(unsigned int irq)
{
        return -EINVAL;
}

The only chance to have the function is in a non-gpiolib platform
that override it.

The irq_to_gpio() in gpio-omap.c overrides this and should
cause compile warnings, does it not?

There is a reason every other driver in drivers/gpio has renamed
this function to things like pxa_irq_to_gpio, msm_irq_to_gpio:
we just don't support it generically.

I know I'm not very helpful, I can just say this is not going to work. :-/

It seems you need a way to get the GPIO corresponding to a certain
IRQ from the device tree, not from the kernel as is done here, then
follow the mentioned sequence.

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 03/22/2013 02:10 AM, Linus Walleij wrote:
> On Fri, Mar 15, 2013 at 12:21 PM, Javier Martinez Canillas
> <martinez.javier@gmail.com> wrote:
> 
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 159f5c5..f5feb43 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -807,6 +807,13 @@ static void gpio_unmask_irq(struct irq_data *d)
>>         spin_unlock_irqrestore(&bank->lock, flags);
>>  }
>>
>> +static int gpio_irq_request(struct irq_data *d)
>> +{
>> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> +
>> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
>> +}
>> +
>>  static struct irq_chip gpio_irq_chip = {
>>         .name           = "GPIO",
>>         .irq_shutdown   = gpio_irq_shutdown,
>> @@ -815,6 +822,7 @@ static struct irq_chip gpio_irq_chip = {
>>         .irq_unmask     = gpio_unmask_irq,
>>         .irq_set_type   = gpio_irq_type,
>>         .irq_set_wake   = gpio_wake_enable,
>> +       .irq_request    = gpio_irq_request,
>>  };
> 
> This is just turning everything on it's head. The normal order of things
> is this sequence for GPIO 14 something like:
> 
> gpio_request(14);
> int irq = gpio_to_irq(14);
> request_any_context_irq(irq);

That doesn't make any sense at all. Drivers don't want to care whether
the IRQ number they receive is a GPIO-that-can-act-like-an-IRQ or a pure
dedicated IRQ input.

To fully support the model you proprose, a driver would have to:

if (gpio_is_valid(pdata->gpio)) {
	gpio_request_one(pdata->gpio, GPIOF_IN, "foo irq");
	irq = gpio_to_irq(pdata->gpio);
} else
	irq = pdata->irq;
request_irq(...);

which means complex code in each driver, plus requiring some indication
in platform data and/or device tree re: whether the IRQ is hosted by a
GPIO or not.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 03/22/2013 10:33 AM, Stephen Warren wrote:
> On 03/22/2013 02:10 AM, Linus Walleij wrote:
>> On Fri, Mar 15, 2013 at 12:21 PM, Javier Martinez Canillas
>> <martinez.javier@gmail.com> wrote:
>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 159f5c5..f5feb43 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -807,6 +807,13 @@ static void gpio_unmask_irq(struct irq_data *d)
>>>         spin_unlock_irqrestore(&bank->lock, flags);
>>>  }
>>>
>>> +static int gpio_irq_request(struct irq_data *d)
>>> +{
>>> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>>> +
>>> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
>>> +}
>>> +
>>>  static struct irq_chip gpio_irq_chip = {
>>>         .name           = "GPIO",
>>>         .irq_shutdown   = gpio_irq_shutdown,
>>> @@ -815,6 +822,7 @@ static struct irq_chip gpio_irq_chip = {
>>>         .irq_unmask     = gpio_unmask_irq,
>>>         .irq_set_type   = gpio_irq_type,
>>>         .irq_set_wake   = gpio_wake_enable,
>>> +       .irq_request    = gpio_irq_request,
>>>  };
>>
>> This is just turning everything on it's head. The normal order of things
>> is this sequence for GPIO 14 something like:
>>
>> gpio_request(14);
>> int irq = gpio_to_irq(14);
>> request_any_context_irq(irq);
> 
> That doesn't make any sense at all. Drivers don't want to care whether
> the IRQ number they receive is a GPIO-that-can-act-like-an-IRQ or a pure
> dedicated IRQ input.
> 
> To fully support the model you proprose, a driver would have to:
> 
> if (gpio_is_valid(pdata->gpio)) {
> 	gpio_request_one(pdata->gpio, GPIOF_IN, "foo irq");
> 	irq = gpio_to_irq(pdata->gpio);
> } else
> 	irq = pdata->irq;
> request_irq(...);
> 
> which means complex code in each driver, plus requiring some indication
> in platform data and/or device tree re: whether the IRQ is hosted by a
> GPIO or not.

I tend to agree with Stephen here. When we had discussed this previously
you had mentioned about adding a platform function to request the gpio
[1], but I could see this adding a bunch more platform files when we are
trying to get rid of all the board-xxx.c files for OMAP. So if you don't
like the approach suggested by Stephen and implemented by Javier, then
may be we need to means to request/reserve gpios in the dts as you had
suggested before [1].

So it seems to me that there are two options at the moment ...

1. Add a irq_chip request as suggested by Stephen.
2. Reserve/request gpios in the dts when registering a gpio chip.

Cheers
Jon

[1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/92327

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Fri, Mar 22, 2013 at 11:52 PM, Jon Hunter <jon-hunter@ti.com> wrote:
> On 03/22/2013 10:33 AM, Stephen Warren wrote:
>> On 03/22/2013 02:10 AM, Linus Walleij wrote:
>>> This is just turning everything on it's head. The normal order of things
>>> is this sequence for GPIO 14 something like:
>>>
>>> gpio_request(14);
>>> int irq = gpio_to_irq(14);
>>> request_any_context_irq(irq);
>>
>> That doesn't make any sense at all. Drivers don't want to care whether
>> the IRQ number they receive is a GPIO-that-can-act-like-an-IRQ or a pure
>> dedicated IRQ input.
>>
>> To fully support the model you proprose, a driver would have to:
>>
>> if (gpio_is_valid(pdata->gpio)) {
>>       gpio_request_one(pdata->gpio, GPIOF_IN, "foo irq");
>>       irq = gpio_to_irq(pdata->gpio);
>> } else
>>       irq = pdata->irq;
>> request_irq(...);
>>
>> which means complex code in each driver, plus requiring some indication
>> in platform data and/or device tree re: whether the IRQ is hosted by a
>> GPIO or not.

I suspect the above is just referring to the DT or similar usecases
(such as ACPI)?

We already have a number of platforms passing IRQ numbers
for something that is actually a GPIO pin but we don't really
know and debugfs doesn't say because they never
issue gpio_request() of the pin. And that's something I
don't like.

Because that is loosing control over the GPIO numberspace
not quite knowing which pin is used and which one isn't
and making these things prone to bugs.

What worries me is changing kernel semantics to fit device tree,
I think it is better to try to confine this to the drivers/gpio/gpiolib-of.c
file.

I'd like OF GPIO code to hog the pin using gpio_request(),
then figure out the IRQ number using gpio_to_irq() and pass
that out when picking an IRQ right out of a GPIO controllers
DT node. And I'd like it to be compulsory exercise to list
that one GPIO line as an input hog when used for just
some IRQ line.

This is what I would want to achieve by the GPIO hog concept.

> I tend to agree with Stephen here. When we had discussed this previously
> you had mentioned about adding a platform function to request the gpio
> [1], but I could see this adding a bunch more platform files when we are
> trying to get rid of all the board-xxx.c files for OMAP. So if you don't
> like the approach suggested by Stephen and implemented by Javier, then
> may be we need to means to request/reserve gpios in the dts as you had
> suggested before [1].
>
> So it seems to me that there are two options at the moment ...
>
> 1. Add a irq_chip request as suggested by Stephen.
> 2. Reserve/request gpios in the dts when registering a gpio chip.

This seems like you need to bring in Grant for a second opinion I'm
getting confused by this now... Paging Alexandre who's doing
the GPIO descriptor refactoring as well.

> [1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/92327

Is there anything wrong with the GPIO hog concept as presented in the
mentioned reply?

+                input-hog-gpios = <4>, <5>;

Would result in these GPIOs being requested, so we can keep
track of them in e.g.  drivers/gpio/gpiolib-of.c, then supply
IRQs from these hogged inputs on demand from drivers.

But with the requirement that they be hogged, so we can keep
track of them, and they show up with some sensible
name in <debugfs>/gpio.

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 03/27/2013 07:52 AM, Linus Walleij wrote:
> On Fri, Mar 22, 2013 at 11:52 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>> On 03/22/2013 10:33 AM, Stephen Warren wrote:
>>> On 03/22/2013 02:10 AM, Linus Walleij wrote:
>>>> This is just turning everything on it's head. The normal order of things
>>>> is this sequence for GPIO 14 something like:
>>>>
>>>> gpio_request(14);
>>>> int irq = gpio_to_irq(14);
>>>> request_any_context_irq(irq);
>>>
>>> That doesn't make any sense at all. Drivers don't want to care whether
>>> the IRQ number they receive is a GPIO-that-can-act-like-an-IRQ or a pure
>>> dedicated IRQ input.
>>>
>>> To fully support the model you proprose, a driver would have to:
>>>
>>> if (gpio_is_valid(pdata->gpio)) {
>>>       gpio_request_one(pdata->gpio, GPIOF_IN, "foo irq");
>>>       irq = gpio_to_irq(pdata->gpio);
>>> } else
>>>       irq = pdata->irq;
>>> request_irq(...);
>>>
>>> which means complex code in each driver, plus requiring some indication
>>> in platform data and/or device tree re: whether the IRQ is hosted by a
>>> GPIO or not.
> 
> I suspect the above is just referring to the DT or similar usecases
> (such as ACPI)?
...
> What worries me is changing kernel semantics to fit device tree,
> I think it is better to try to confine this to the drivers/gpio/gpiolib-of.c
> file.

No, this has absolutely noting to do with device tree; the example I
gave above is purely based on platform data.

It's simply that if a device emits an IRQ, there's no reason to assume
that the IRQ is in fact a GPIO. It might be a dedicated IRQ input pin
and not something that gpiolib (or any other GPIO API) knows about.

One possibility:

(Device IRQ output wired into GPIO input on SoC which then generates an
interrupt)

+----------------------------+
| SoC                        |         +--------+
|     IRQ 5 +------+ GPIO 10 | DEV_IRQ | Device |
| GIC <---- | GPIO | <-------|---------|-IRQ    |
|           +------+         |         +--------+
+----------------------------+

Another possibility:

(Device IRQ output wired directly into dedicated IRQ input pin, and that
pin has no GPIO capabilities)

+----------------------------+
| SoC                        |         +--------+
|      IRQ 5                 | DEV_IRQ | Device |
| GIC <----------------------|---------|-IRQ    |
|                            |         +--------+
+----------------------------+

As such, the driver /must/ receive an "int irq" in platform data. If it
received an "int gpio", then there would be no way for the driver to
support boards that routed that interrupt signal to a dedicated IRQ pin
on the SoC, rather than to a GPIO pin that just happened to have
interrupt generation capabilities.

And then, given that irq_to_gpio() isn't semantically possible, there's
no way for the driver to be able to gpio_request() anything, since it
doesn't, can't, and shouldn't know whether there is in fact any GPIO ID
that it should request, let alone what that GPIO ID is.

And finally, even if we ignore dedicated IRQ input pins, and assume that
every IRQ input is really a GPIO, why should the driver be forced to
call both request_irq() and gpio_request(); that's something that should
really be dealt with inside the IRQ subsystem or relevant IRQ driver.
Otherwise, every other driver that uses IRQs has to be burdened with the
code to do that.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> It's simply that if a device emits an IRQ, there's no reason to assume
> that the IRQ is in fact a GPIO. It might be a dedicated IRQ input pin
> and not something that gpiolib (or any other GPIO API) knows about.

OK (following the first example, that's what I have in the Nomadik
or Snowball already).

> Another possibility:
>
> (Device IRQ output wired directly into dedicated IRQ input pin, and that
> pin has no GPIO capabilities)
>
> +----------------------------+
> | SoC                        |         +--------+
> |      IRQ 5                 | DEV_IRQ | Device |
> | GIC <----------------------|---------|-IRQ    |
> |                            |         +--------+
> +----------------------------+
>
> As such, the driver /must/ receive an "int irq" in platform data.

Nah. Lets pass a struct resource * of type IORESOURCE_IRQ.
But I understand the necessity to pass the IRQ number somehow.

> If it
> received an "int gpio", then there would be no way for the driver to
> support boards that routed that interrupt signal to a dedicated IRQ pin
> on the SoC, rather than to a GPIO pin that just happened to have
> interrupt generation capabilities.

This is the case for some SMSC911x clients like the snowball
routing it to a GPIO, whereas I think the RealView and Integrator/CP
has a dedicated IRQ line for it on the FPGA so it's a good example.

In such cases the right thing to do is for the platform
code populating the platform data with the int irq field, or device tree
core, or whatever piece of code that knows about
the actual GPIO shall start from the GPIO, request it and
then convert it to an IRQ.

If it seems like identical boilerplate in every machine we can
simply centralize it to gpiolib. Something like

int gpio_add_single_irq_resource(struct platform_device *pdev, int
gpio, unsigned flags)
{
     struct resource *r;
     int irq;
     int err;

     r = devm_kmalloc(&pdev->dev, sizeof(*r), GFP_KERNEL);
     if (!r)
        return -ENOMEM;
     memset(r, 0, sizeof(*r));
     err = request_gpio(gpio);
     if (err)
        return err;
     irq = gpio_to_irq(gpio);
     if (irq < 0)
        return irq;
     r->start = irq;
     r->end = irq;
     r->flags = IORESOURCE_IRQ | flags;
     pdev->num_resources = 1;
     pdev->resource = res;
};

int gpio_populate_irq_resource(struct platform_device *pdev, int gpio,
                              unsigned flags, struct resource *r)
{
     int irq;
     int err;

     err = request_gpio(gpio);
     if (err)
        return err;
     irq = gpio_to_irq(gpio);
     if (irq < 0)
        return irq;
     r->start = irq;
     r->end = irq;
     r->flags = IORESOURCE_IRQ | flags;
};


(Add permutations for either filling in a struct resource *r from
the board code, or krealloc() to extend an existing dynamic allocation
or whatever, surely these helpers can be written.)

So this is for a pure platform data case (based on
arch/arm/mach-ux500/board-mop500.c, which currently
relies on hard-coded GPIO and IRQ numbers):

static struct resource ethernet_res[] = {
        {
                .name = "smsc911x-memory",
                .start = (0x5000 << 16),
                .end  =  (0x5000 << 16) + 0xffff,
                .flags = IORESOURCE_MEM,
        },
        {  }, /* To be filled in with GPIO IRQ */
};

static struct platform_device ethernet = {
        .name           = "smsc911x",
        .dev            = {
                .platform_data = &....,
        },
};

gpio_populate_irq_resource(&ethernet, 17,
     IORESOURCE_IRQ_HIGHEDGE, &ethernet_res[1]);
platform_device_register(&ethernet);

That populates the device with the single GPIO IRQ from
the GPIO number in a smooth way.

Of course it has to be done after the GPIO driver is up
and running so may require some other refactoring,
but it should work.

I can implement this and convert the Snowball if it
helps.

> And then, given that irq_to_gpio() isn't semantically possible, there's
> no way for the driver to be able to gpio_request() anything, since it
> doesn't, can't, and shouldn't know whether there is in fact any GPIO ID
> that it should request, let alone what that GPIO ID is.

That's cool. The driver does not have to know the IRQ is backed by a
GPIO, but somewhere in the system something should know this.
Such as board code or DT core (I'm thinking drivers/of/platform.c code
calling gpiolib-of.c to do this mapping for DT, for example).

And by introducing the requirement that such GPIO lines be
GPIO input hogs in the device tree else warn we can get a nice
closure in the DT case: the GPIO subsystem knows the actual
GPIO line as a hog that is request and sets as input at probe,
the referencing device node only gets the IRQ but at runtime
the device node instatiation for the IRQ consumer will ask
the gpiolib to check its hog list to make sure one of these will
match that IRQ, then request it.

> And finally, even if we ignore dedicated IRQ input pins, and assume that
> every IRQ input is really a GPIO, why should the driver be forced to
> call both request_irq() and gpio_request(); that's something that should
> really be dealt with inside the IRQ subsystem or relevant IRQ driver.
> Otherwise, every other driver that uses IRQs has to be burdened with the
> code to do that.

I agree it need not be in the driver if all it wants is an IRQ.

I was thinking more like the above.

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 03/27/2013 02:55 PM, Linus Walleij wrote:
> On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> It's simply that if a device emits an IRQ, there's no reason to assume
>> that the IRQ is in fact a GPIO. It might be a dedicated IRQ input pin
>> and not something that gpiolib (or any other GPIO API) knows about.
> 
> OK (following the first example, that's what I have in the Nomadik
> or Snowball already).
> 
>> Another possibility:
>>
>> (Device IRQ output wired directly into dedicated IRQ input pin, and that
>> pin has no GPIO capabilities)
>>
>> +----------------------------+
>> | SoC                        |         +--------+
>> |      IRQ 5                 | DEV_IRQ | Device |
>> | GIC <----------------------|---------|-IRQ    |
>> |                            |         +--------+
>> +----------------------------+
>>
>> As such, the driver /must/ receive an "int irq" in platform data.
> 
> Nah. Lets pass a struct resource * of type IORESOURCE_IRQ.
> But I understand the necessity to pass the IRQ number somehow.

Oh sure. I tend to consider the resources part of platform data,
although I agree they really aren't.

>> If it
>> received an "int gpio", then there would be no way for the driver to
>> support boards that routed that interrupt signal to a dedicated IRQ pin
>> on the SoC, rather than to a GPIO pin that just happened to have
>> interrupt generation capabilities.
> 
> This is the case for some SMSC911x clients like the snowball
> routing it to a GPIO, whereas I think the RealView and Integrator/CP
> has a dedicated IRQ line for it on the FPGA so it's a good example.
> 
> In such cases the right thing to do is for the platform
> code populating the platform data with the int irq field, or device tree
> core, or whatever piece of code that knows about
> the actual GPIO shall start from the GPIO, request it and
> then convert it to an IRQ.

So board code could easily do that; plenty of opportunity to put
whatever custom code is needed right there.

For DT core code to do that, we'd need some alternative way of
specifying interrupts. That would change the DT bindings for interrupts.
I don't think we can do that...

> If it seems like identical boilerplate in every machine we can
> simply centralize it to gpiolib. Something like
> 
> int gpio_add_single_irq_resource(struct platform_device *pdev, int
> gpio, unsigned flags)
... [code to convert GPIO to IRQ and create IORESOURCE_IRQ from it]

> int gpio_populate_irq_resource(struct platform_device *pdev, int gpio,
>                               unsigned flags, struct resource *r)
... [code to create IORESOURCE_IRQ from the IRQ]

...
> gpio_populate_irq_resource(&ethernet, 17,
>      IORESOURCE_IRQ_HIGHEDGE, &ethernet_res[1]);
> platform_device_register(&ethernet);
> 
> That populates the device with the single GPIO IRQ from
> the GPIO number in a smooth way.
> 
> Of course it has to be done after the GPIO driver is up
> and running so may require some other refactoring,
> but it should work.

That latter issue also somewhat scuppers this approach; then you have to
somehow run a bunch of your board file code inter-mixed with various
driver probe() calls. That will quickly get nasy.

And it doesn't address how the DT core will know when to call
gpio_add_single_irq_resource() vs. gpio_populate_irq_resource() without
changing the DT binding for interrupts.

>> And then, given that irq_to_gpio() isn't semantically possible, there's
>> no way for the driver to be able to gpio_request() anything, since it
>> doesn't, can't, and shouldn't know whether there is in fact any GPIO ID
>> that it should request, let alone what that GPIO ID is.
> 
> That's cool. The driver does not have to know the IRQ is backed by a
> GPIO, but somewhere in the system something should know this.

Something already does; the irqchip driver for the IRQ in question would
always know whether it's programming a plain IRQ controller, or an IRQ
controller that's part of a GPIO controller. In the GPIO case, the
irqchip driver also has enought HW-specific information to perform
whatever GPIO<->IRQ number conversion is needed.

> Such as board code or DT core (I'm thinking drivers/of/platform.c code
> calling gpiolib-of.c to do this mapping for DT, for example).
> 
> And by introducing the requirement that such GPIO lines be
> GPIO input hogs in the device tree else warn we can get a nice
> closure in the DT case: the GPIO subsystem knows the actual
> GPIO line as a hog that is request and sets as input at probe,
> the referencing device node only gets the IRQ but at runtime
> the device node instatiation for the IRQ consumer will ask
> the gpiolib to check its hog list to make sure one of these will
> match that IRQ, then request it.

"Hogs" sounds like pinctrl. pinctrl doesn't support the concept of hogs
on GPIOs, since it only knows about pins not GPIOs really; even with the
integration with gpiolib, pinctrl gets informed when GPIOs are
requested, not when setting up pinctrl states ahead of time, and also
this notification is only so we can mark the pin related to the GPIO
in-use, not really to actually hog it. If pinctrl started hogging GPIOs,
I think me might end up with both pinctrl and gpiolib calling
each-other, and hence have nasty circular dependencies.

Or, were you thinking of creating some kind of hogging system for
gpiolib itself?

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Fri, Mar 29, 2013 at 6:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/27/2013 02:55 PM, Linus Walleij wrote:
>> On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>> This is the case for some SMSC911x clients like the snowball
>> routing it to a GPIO, whereas I think the RealView and Integrator/CP
>> has a dedicated IRQ line for it on the FPGA so it's a good example.
>>
>> In such cases the right thing to do is for the platform
>> code populating the platform data with the int irq field, or device tree
>> core, or whatever piece of code that knows about
>> the actual GPIO shall start from the GPIO, request it and
>> then convert it to an IRQ.
>
> So board code could easily do that; plenty of opportunity to put
> whatever custom code is needed right there.
>
> For DT core code to do that, we'd need some alternative way of
> specifying interrupts. That would change the DT bindings for interrupts.
> I don't think we can do that...

Sorry, I'm probably not knowledgeable about DT to understand this.
The information for what GPIO corresponds to what IRQ is
present in the device tree, is it not?

If the information is there, whether to convert from IRQ to GPIO
or from GPIO to IRQ is a technicality and any order should be
feasible in some way?

I just can't get the necessity to do it one way preferred over the
other through my head, sorry...

>> If it seems like identical boilerplate in every machine we can
>> simply centralize it to gpiolib. Something like
>>
>> int gpio_add_single_irq_resource(struct platform_device *pdev, int
>> gpio, unsigned flags)
> ... [code to convert GPIO to IRQ and create IORESOURCE_IRQ from it]
>
>> int gpio_populate_irq_resource(struct platform_device *pdev, int gpio,
>>                               unsigned flags, struct resource *r)
> ... [code to create IORESOURCE_IRQ from the IRQ]
>
> ...
>> gpio_populate_irq_resource(&ethernet, 17,
>>      IORESOURCE_IRQ_HIGHEDGE, &ethernet_res[1]);
>> platform_device_register(&ethernet);
>>
>> That populates the device with the single GPIO IRQ from
>> the GPIO number in a smooth way.
>>
>> Of course it has to be done after the GPIO driver is up
>> and running so may require some other refactoring,
>> but it should work.
>
> That latter issue also somewhat scuppers this approach; then you have to
> somehow run a bunch of your board file code inter-mixed with various
> driver probe() calls. That will quickly get nasy.

No, just use a module_init() for the above code in the board
file and it will defer just like any other initcall.

> And it doesn't address how the DT core will know when to call
> gpio_add_single_irq_resource() vs. gpio_populate_irq_resource() without
> changing the DT binding for interrupts.

Is it not possible to do this in
drivers/gpio/gpiolib-of.c: of_gpiochip_add(), if the DT *know*
which GPIOs will be used as plain IRQs? That is the point with
the gpio hogs I talk about ...

GPIO hogs would hog input pins here and immediately
call gpio_to_irq() on them, and from that point on they can
only be used for input, and only for generating IRQs.

>>> And then, given that irq_to_gpio() isn't semantically possible, there's
>>> no way for the driver to be able to gpio_request() anything, since it
>>> doesn't, can't, and shouldn't know whether there is in fact any GPIO ID
>>> that it should request, let alone what that GPIO ID is.
>>
>> That's cool. The driver does not have to know the IRQ is backed by a
>> GPIO, but somewhere in the system something should know this.
>
> Something already does; the irqchip driver for the IRQ in question would
> always know whether it's programming a plain IRQ controller, or an IRQ
> controller that's part of a GPIO controller. In the GPIO case, the
> irqchip driver also has enought HW-specific information to perform
> whatever GPIO<->IRQ number conversion is needed.

So I'm opposed to doing these things both ways because
I think it's getting messy. We have already has irq_to_gpio()
cause problems in the past and this reminds me heavily
about that.

> Or, were you thinking of creating some kind of hogging system for
> gpiolib itself?

Yes.

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/10/2013 12:12 PM, Linus Walleij wrote:
> On Fri, Mar 29, 2013 at 6:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 03/27/2013 02:55 PM, Linus Walleij wrote:
>>> On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>
>>> This is the case for some SMSC911x clients like the snowball
>>> routing it to a GPIO, whereas I think the RealView and Integrator/CP
>>> has a dedicated IRQ line for it on the FPGA so it's a good example.
>>>
>>> In such cases the right thing to do is for the platform
>>> code populating the platform data with the int irq field, or device tree
>>> core, or whatever piece of code that knows about
>>> the actual GPIO shall start from the GPIO, request it and
>>> then convert it to an IRQ.
>>
>> So board code could easily do that; plenty of opportunity to put
>> whatever custom code is needed right there.
>>
>> For DT core code to do that, we'd need some alternative way of
>> specifying interrupts. That would change the DT bindings for interrupts.
>> I don't think we can do that...
> 
> Sorry, I'm probably not knowledgeable about DT to understand this.
> The information for what GPIO corresponds to what IRQ is
> present in the device tree, is it not?

No, I don't think so. DT treats the IRQ and GPIO numbering spaces as
entirely unrelated things, just like the kernel APIs do.

The fact that some DT nodes (device/drivers) happen to be both GPIO and
IRQ providers (implement gpio_chip and irq_chip) and happen to use the
same numbering scheme for both GPIO and IRQ IDs is purely an
implementation detail within individual drivers (or rather, of specific
DT binding definitions really), and not a general truism.

> If the information is there, whether to convert from IRQ to GPIO
> or from GPIO to IRQ is a technicality and any order should be
> feasible in some way?

There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put
another way, a single GPIO would likely only ever trigger a single IRQ,
but a single IRQ might easily be triggered by any number of GPIOs. This
is exactly why the function irq_to_gpio() isn't something one should use
any more. I think there was an effort to outright remove the API,
although it doesn't look like that's entirely complete yet. I guess you
know this given your mentions of problem with gpio_to_irq() later on, so
I'm not quite sure what your paragraph above was supposed to mean.

> I just can't get the necessity to do it one way preferred over the
> other through my head, sorry...
> 
>>> If it seems like identical boilerplate in every machine we can
>>> simply centralize it to gpiolib. Something like
>>>
>>> int gpio_add_single_irq_resource(struct platform_device *pdev, int
>>> gpio, unsigned flags)
>> ... [code to convert GPIO to IRQ and create IORESOURCE_IRQ from it]
>>
>>> int gpio_populate_irq_resource(struct platform_device *pdev, int gpio,
>>>                               unsigned flags, struct resource *r)
>> ... [code to create IORESOURCE_IRQ from the IRQ]
>>
>> ...
>>> gpio_populate_irq_resource(&ethernet, 17,
>>>      IORESOURCE_IRQ_HIGHEDGE, &ethernet_res[1]);
>>> platform_device_register(&ethernet);
>>>
>>> That populates the device with the single GPIO IRQ from
>>> the GPIO number in a smooth way.
>>>
>>> Of course it has to be done after the GPIO driver is up
>>> and running so may require some other refactoring,
>>> but it should work.
>>
>> That latter issue also somewhat scuppers this approach; then you have to
>> somehow run a bunch of your board file code inter-mixed with various
>> driver probe() calls. That will quickly get nasy.
> 
> No, just use a module_init() for the above code in the board
> file and it will defer just like any other initcall.

Using initcalls to order things is rather fragile. Perhaps it could work
out OK with board files, but it's certainly something that's frowned
upon now for DT-based systems, since deferred probe solves it much more
generally, and without any maintenance issues (e.g. continual
re-shuffling of the initcall order, not having enough initcall levels if
you end up with more dependencies being added, etc.).

>> And it doesn't address how the DT core will know when to call
>> gpio_add_single_irq_resource() vs. gpio_populate_irq_resource() without
>> changing the DT binding for interrupts.
> 
> Is it not possible to do this in
> drivers/gpio/gpiolib-of.c: of_gpiochip_add(), if the DT *know*
> which GPIOs will be used as plain IRQs? That is the point with
> the gpio hogs I talk about ...

I suppose that if you modified the device tree bindings (schema
definitions), you could have every GPIO provider DT node indicate which
GPIOs were used as IRQs instead. However,

a) This would be an incompatible change to the DT bindings, and they're
supposed to be a stable ABI; old DTBs should work unmodified with new
kernels.

b) This would place GPIO usage information in multiple places. Right
now, GPIO consumers list which GPIO(s) they use. The same goes for IRQs.
If a GPIO provider started listing which GPIOs were really IRQs, then
that'd mean configuring each GPIO-as-an-IRQ in two places; once in the
client node to indicate which IRQ it should hook/register, and once in
the provider node to indicate that the GPIO is really an IRQ. Two places
is more hassle for people to implement, and more opportunity for mistakes.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Wed, Apr 10, 2013 at 10:29 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/10/2013 12:12 PM, Linus Walleij wrote:

>> If the information is there, whether to convert from IRQ to GPIO
>> or from GPIO to IRQ is a technicality and any order should be
>> feasible in some way?
>
> There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put
> another way, a single GPIO would likely only ever trigger a single IRQ,
> but a single IRQ might easily be triggered by any number of GPIOs. This
> is exactly why the function irq_to_gpio() isn't something one should use
> any more. I think there was an effort to outright remove the API,
> although it doesn't look like that's entirely complete yet. I guess you
> know this given your mentions of problem with gpio_to_irq() later on, so
> I'm not quite sure what your paragraph above was supposed to mean.

So the only reason I'm rambing on about this is that it breaks the
connection between GPIOs and their IRQs and at one point
I percieved it as there was going to be some IRQ -> GPIO lookup,
and it would sneak back in. But now I realize that may have been
supposed to be something local to the driver, in it's irqchip portions
and then it's actually no problem for the kernel at large.

Let's restate: I'm also after something fragile here.

IIUC, it will be possible for a GPIO to be set up as input and used
as an IRQ without the GPIO subsystem knowing it. And it will be
possible for the GPIO subsystem to go in and request the same pin
and set it as output and e.g. bit-bang it. I wonder what happens then.

If the drive code is written in such a way that this will be denied,
I'll soften up, but I'd like to see the code first.

Then the same mistake has to be avoided in all drivers.

And that makes me want to probe for solutions tying them
up in the core so no mistakes can be made.

>> Is it not possible to do this in
>> drivers/gpio/gpiolib-of.c: of_gpiochip_add(), if the DT *know*
>> which GPIOs will be used as plain IRQs? That is the point with
>> the gpio hogs I talk about ...
>
> I suppose that if you modified the device tree bindings (schema
> definitions), you could have every GPIO provider DT node indicate which
> GPIOs were used as IRQs instead. However,
>
> a) This would be an incompatible change to the DT bindings, and they're
> supposed to be a stable ABI; old DTBs should work unmodified with new
> kernels.
>
> b) This would place GPIO usage information in multiple places. Right
> now, GPIO consumers list which GPIO(s) they use. The same goes for IRQs.
> If a GPIO provider started listing which GPIOs were really IRQs, then
> that'd mean configuring each GPIO-as-an-IRQ in two places; once in the
> client node to indicate which IRQ it should hook/register, and once in
> the provider node to indicate that the GPIO is really an IRQ. Two places
> is more hassle for people to implement, and more opportunity for mistakes.

Yeah but the current approach AFAICT isn't exactly safe either.

So I need some more convincing... or code rather, I guess.
If I can't point out the problems I see in a piece of code I guess
I'm just plain wrong :-)

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Arnd Bergmann]

On Wednesday 10 April 2013 14:29:17 Stephen Warren wrote:
> 
> > If the information is there, whether to convert from IRQ to GPIO
> > or from GPIO to IRQ is a technicality and any order should be
> > feasible in some way?
> 
> There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put
> another way, a single GPIO would likely only ever trigger a single IRQ,
> but a single IRQ might easily be triggered by any number of GPIOs. This
> is exactly why the function irq_to_gpio() isn't something one should use
> any more.

More importantly, most irqs don't have any GPIO associated with them
at all. The interface made some sense for simple platforms that had
a linear range of GPIO IRQs, but that also isn't true these days,
with arbitrary IRQ domains.

> I think there was an effort to outright remove the API,
> although it doesn't look like that's entirely complete yet.

It's essentially complete. There are a few remnants in areas of the kernel
that we don't like to look at:

* The blackfin architecture provides the interface and uses it in the
  pata_rb532_cf driver.

* MIPS provides it on a few older platforms, and it's used on the alchemy
  pcmcia driver.

* avr32, m68k, sh and unicore32 provide the interface but don't use it.

* On ARM, the interface is provided on iop, gemini, ixp4xx, ks8695
  and w90x900.  None of these use it, and they rarely see any patches
  at all these days.

* ARM/davinci provides a stub but always return -ENOSYS.

* One driver for PXA (tosa_battery) still uses this interface, but PXA stopped
  providing it long ago.

	Arnd

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/10/2013 03:28 PM, Linus Walleij wrote:
> On Wed, Apr 10, 2013 at 10:29 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/10/2013 12:12 PM, Linus Walleij wrote:
> 
>>> If the information is there, whether to convert from IRQ to GPIO
>>> or from GPIO to IRQ is a technicality and any order should be
>>> feasible in some way?
>>
>> There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put
>> another way, a single GPIO would likely only ever trigger a single IRQ,
>> but a single IRQ might easily be triggered by any number of GPIOs. This
>> is exactly why the function irq_to_gpio() isn't something one should use
>> any more. I think there was an effort to outright remove the API,
>> although it doesn't look like that's entirely complete yet. I guess you
>> know this given your mentions of problem with gpio_to_irq() later on, so
>> I'm not quite sure what your paragraph above was supposed to mean.
> 
> So the only reason I'm rambing on about this is that it breaks the

I'm not sure I understand this paragraph; what is "it" in the line above.

If "it" is this patch, then should "breaks" be re-establishes?

> connection between GPIOs and their IRQs and at one point
> I percieved it as there was going to be some IRQ -> GPIO lookup,
> and it would sneak back in. But now I realize that may have been
> supposed to be something local to the driver, in it's irqchip portions
> and then it's actually no problem for the kernel at large.

Yes, I believe the intention was for their to be a correlation between
GPIOs and IRQs only with the individual gpio_chip/irq_chip drivers for
those GPIOs/IRQs, and not exposed anywhere outside it.

> Let's restate: I'm also after something fragile here.

I assume you mean the opposite of that?

> IIUC, it will be possible for a GPIO to be set up as input and used
> as an IRQ without the GPIO subsystem knowing it. And it will be
> possible for the GPIO subsystem to go in and request the same pin
> and set it as output and e.g. bit-bang it. I wonder what happens then.

Yes, I think that's possible now.

If I recall the patch I'm replying to correctly, the idea was to add an
"IRQ request" operation that would (internally to the GPIO/IRQ driver
itself) map IRQ->GPIO, and call gpio_request(). That would then prevent
exactly the situation you mention above.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/10/2013 03:28 PM, Linus Walleij wrote:

>> So the only reason I'm rambing on about this is that it breaks the
>
> I'm not sure I understand this paragraph; what is "it" in the line above.
>
> If "it" is this patch, then should "breaks" be re-establishes?

No I'm replying to Javier Martinez Canillas mail in this thread:
http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2
which is stating a question to grand, and contains the below
hunk:

> +static int gpio_irq_request(struct irq_data *d)
> +{
> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +
> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
> +}

irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().

It's the same thing you mention below:

> If I recall the patch I'm replying to correctly, the idea was to add an
> "IRQ request" operation that would (internally to the GPIO/IRQ driver
> itself) map IRQ->GPIO, and call gpio_request(). That would then prevent
> exactly the situation you mention above.

So the above does not look like it's internal to the driver does it?

I think this is irq_to_gpio sneaking back in, and requiring that every
driver of this sort follow the same design pattern. And then maybe
you see my persistance ... are we talking about different things?

So I'd be happy with something local to the driver, foo_irq_to_gpio()
and that we also back out a bit and see what the subsystem can do
to avoid this kind of code having to be duplicated everywhere, that's
all.

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/11/2013 04:16 PM, Linus Walleij wrote:
> On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/10/2013 03:28 PM, Linus Walleij wrote:
> 
>>> So the only reason I'm rambing on about this is that it breaks the
>>
>> I'm not sure I understand this paragraph; what is "it" in the line above.
>>
>> If "it" is this patch, then should "breaks" be re-establishes?
> 
> No I'm replying to Javier Martinez Canillas mail in this thread:
> http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2
> which is stating a question to grand, and contains the below
> hunk:
> 
>> +static int gpio_irq_request(struct irq_data *d)
>> +{
>> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> +
>> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
>> +}
> 
> irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().

OK, right. sorry for being so confused/confusing.

Yes, that code should certainly call e.g. omap_gpio__irq_to_gpio() not
irq_to_gpio(). Probably gpio_irq_request() wants renaming to something
more like omap_gpio__irq_request() too, so the names don't look like
they'd clash with global functions. (__ added for clarity but probably
only one _ at a time)

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Fri, Apr 12, 2013 at 12:16 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/10/2013 03:28 PM, Linus Walleij wrote:
>
>>> So the only reason I'm rambing on about this is that it breaks the
>>
>> I'm not sure I understand this paragraph; what is "it" in the line above.
>>
>> If "it" is this patch, then should "breaks" be re-establishes?
>
> No I'm replying to Javier Martinez Canillas mail in this thread:
> http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2
> which is stating a question to grand, and contains the below
> hunk:
>
>> +static int gpio_irq_request(struct irq_data *d)
>> +{
>> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> +
>> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
>> +}
>
> irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().
>
> It's the same thing you mention below:
>
>> If I recall the patch I'm replying to correctly, the idea was to add an
>> "IRQ request" operation that would (internally to the GPIO/IRQ driver
>> itself) map IRQ->GPIO, and call gpio_request(). That would then prevent
>> exactly the situation you mention above.
>
> So the above does not look like it's internal to the driver does it?
>
> I think this is irq_to_gpio sneaking back in, and requiring that every
> driver of this sort follow the same design pattern. And then maybe
> you see my persistance ... are we talking about different things?
>
> So I'd be happy with something local to the driver, foo_irq_to_gpio()
> and that we also back out a bit and see what the subsystem can do
> to avoid this kind of code having to be duplicated everywhere, that's
> all.
>

Hi Linus,

Thanks a lot for your explanation. I didn't know that irq_to_gpio()
was being deprecated and we shouldn't use anymore. Now from this
thread discussion I understand the reasons for this decision.

I'll read the whole thread again and try to implement something that
is local to the gpio-omap driver instead using irq_to_gpio().

Besides using a controller specific mapping instead of irq_to_gpio(),
do you think that is a good idea to add a new "irq_request" function
pointer to struct irq_chip?

The idea is that GPIO controller drivers can call gpio_request() and
enabling the GPIO bank module before a call to request_irq() is made.
Or do you think that is better to implement the DT gpio hogs that you
were suggesting?

I really want to find a solution to this since currently we can't use
any device that uses an GPIO line as an IRQ on OMAP based boards.

> Yours,
> Linus Walleij

Best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/11/2013 04:16 PM, Linus Walleij wrote:
> On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/10/2013 03:28 PM, Linus Walleij wrote:
> 
>>> So the only reason I'm rambing on about this is that it breaks the
>>
>> I'm not sure I understand this paragraph; what is "it" in the line above.
>>
>> If "it" is this patch, then should "breaks" be re-establishes?
> 
> No I'm replying to Javier Martinez Canillas mail in this thread:
> http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2
> which is stating a question to grand, and contains the below
> hunk:
> 
>> +static int gpio_irq_request(struct irq_data *d)
>> +{
>> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> +
>> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
>> +}
> 
> irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().
> 
> It's the same thing you mention below:
> 
>> If I recall the patch I'm replying to correctly, the idea was to add an
>> "IRQ request" operation that would (internally to the GPIO/IRQ driver
>> itself) map IRQ->GPIO, and call gpio_request(). That would then prevent
>> exactly the situation you mention above.

OK, right. sorry for being so confused/confusing.

Yes, that code should certainly call e.g. omap_gpio__irq_to_gpio() not
irq_to_gpio(). Probably gpio_irq_request() wants renaming to something
more like omap_gpio__irq_request() too, so the names don't look like
they'd clash with global functions. (__ added for clarity but probably
only one _ at a time)

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Fri, Apr 12, 2013 at 12:47 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/11/2013 04:16 PM, Linus Walleij wrote:
>> On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 04/10/2013 03:28 PM, Linus Walleij wrote:
>>
>>>> So the only reason I'm rambing on about this is that it breaks the
>>>
>>> I'm not sure I understand this paragraph; what is "it" in the line above.
>>>
>>> If "it" is this patch, then should "breaks" be re-establishes?
>>
>> No I'm replying to Javier Martinez Canillas mail in this thread:
>> http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2
>> which is stating a question to grand, and contains the below
>> hunk:
>>
>>> +static int gpio_irq_request(struct irq_data *d)
>>> +{
>>> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>>> +
>>> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
>>> +}
>>
>> irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().
>
> OK, right. sorry for being so confused/confusing.
>
> Yes, that code should certainly call e.g. omap_gpio__irq_to_gpio() not
> irq_to_gpio(). Probably gpio_irq_request() wants renaming to something
> more like omap_gpio__irq_request() too, so the names don't look like
> they'd clash with global functions. (__ added for clarity but probably
> only one _ at a time)

Stephen, Linus,

Is the following inlined patch [1] what you were thinking that would
be the right approach?

With this patch an explicit call to call gpio_request() before a call
to chip->irq_set_type() is needed. I've tested both with DT and
without DT where a explicit call to gpio_request() is made and it
works in both cases. So it shouldn't have a functional change for
non-DT cases as far as I know.

If you agree with [1] then I'll split in two patches (one that adds
the irq_request function pointer to irq_chip and another one that adds
the implementation for gpio-omap) and send as a patch-set. I just
thought that it would be easier for you if I posted here an inlined
patch so you could have context.

Thanks a lot for your feedback and best regards,
Javier

[1]
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 685e850..e035e64 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -811,6 +811,13 @@ static void gpio_unmask_irq(struct irq_data *d)
 	spin_unlock_irqrestore(&bank->lock, flags);
 }

+static int omap_gpio_irq_request(struct irq_data *d)
+{
+	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+	return omap_gpio_request(&bank->chip, d->hwirq);
+}
+
 static struct irq_chip gpio_irq_chip = {
 	.name		= "GPIO",
 	.irq_shutdown	= gpio_irq_shutdown,
@@ -819,6 +826,7 @@ static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.irq_request    = omap_gpio_irq_request,
 };

 /*---------------------------------------------------------------------*/
@@ -1028,6 +1036,7 @@ omap_mpuio_alloc_gc(struct gpio_bank *bank,
unsigned int irq_start,
 	ct->chip.irq_mask = irq_gc_mask_set_bit;
 	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
 	ct->chip.irq_set_type = gpio_irq_type;
+	ct->chip.irq_request = omap_gpio_irq_request;

 	if (bank->regs->wkup_en)
 		ct->chip.irq_set_wake = gpio_wake_enable,
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 0e8e3a6..85596cc 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -303,6 +303,7 @@ struct irq_chip {
 	void		(*irq_shutdown)(struct irq_data *data);
 	void		(*irq_enable)(struct irq_data *data);
 	void		(*irq_disable)(struct irq_data *data);
+	int             (*irq_request)(struct irq_data *data);

 	void		(*irq_ack)(struct irq_data *data);
 	void		(*irq_mask)(struct irq_data *data);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..a4bd4f7 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -588,6 +588,12 @@ int __irq_set_trigger(struct irq_desc *desc,
unsigned int irq,
 			unmask = 1;
 	}

+	if (chip->irq_request) {
+			ret = chip->irq_request(&desc->irq_data);
+			if (ret)
+				return ret;
+	}
+
 	/* caller masked out all except trigger mode flags */
 	ret = chip->irq_set_type(&desc->irq_data, flags);

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
<martinez.javier@gmail.com> wrote:

> Is the following inlined patch [1] what you were thinking that would
> be the right approach?

This looks sort of OK, but I'm still struggling with the question of
what we could do to help other implementations facing the same issue.

This is a pretty hard design pattern to properly replicate in every such
driver is it not?

Hmmmm

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Sun, Apr 14, 2013 at 10:53 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
> <martinez.javier@gmail.com> wrote:
>
>> Is the following inlined patch [1] what you were thinking that would
>> be the right approach?

Hi Linus, thanks a lot for your feedback.

>
> This looks sort of OK, but I'm still struggling with the question of
> what we could do to help other implementations facing the same issue.
>

Yes, I don't know how we can make it easier to other implementations
(besides adding the irq_request hook to irq_chip) since the logic is
GPIO controller specific.

For example I took a look to drivers/gpio/gpio-tegra.c and if I
understood correctly, the implementation for this driver should be
something like this:

diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 414ad91..a2d5c3d 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -346,6 +346,11 @@ static int tegra_gpio_wake_enable(struct irq_data
*d, unsigned int enable)
 }
 #endif

+static int tegra_gpio_irq_request(struct irq_data *d)
+{
+	tegra_gpio_request(NULL, d->hwirq);
+}
+
 static struct irq_chip tegra_gpio_irq_chip = {
 	.name		= "GPIO",
 	.irq_ack	= tegra_gpio_irq_ack,
@@ -355,6 +360,7 @@ static struct irq_chip tegra_gpio_irq_chip = {
 #ifdef CONFIG_PM_SLEEP
 	.irq_set_wake	= tegra_gpio_wake_enable,
 #endif
+	.irq_request    = tegra_gpio_irq_request,
 };

 static const struct dev_pm_ops tegra_gpio_pm_ops = {

This is definitely quite similar to the omap-gpio.c change but not the same.

> This is a pretty hard design pattern to properly replicate in every such
> driver is it not?
>
> Hmmmm
>

Is indeed a hard design pattern but I couldn't figure out a more
generic way to handle this.

Maybe we could use [1] for now to solve the issue that we have with
OMAP GPIO and later when the same issue is found on other GPIO
controllers and a similar change is made on these drivers we will see
some form of pattern that emerge allowing us to make a later refactor
to reduce the code duplication.

Or do you have a better idea on how to solve this?

> Yours,
> Linus Walleij

Best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/13/2013 07:35 PM, Javier Martinez Canillas wrote:
...
> Is the following inlined patch [1] what you were thinking that would
> be the right approach?
...
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
...
> +static int omap_gpio_irq_request(struct irq_data *d)
> +{
> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +
> +	return omap_gpio_request(&bank->chip, d->hwirq);

If you want the GPIO usage to be known to the GPIO subsystem, then
wouldn't you call gpio_request() here rather than omap_gpio_request()?
The above code will certainly do enough so that the OMAP GPIO HW is
fully enabled as you need, but I thought the idea was to also prevent
some other code successfully running gpio_request() on that same GPIO?

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/14/2013 02:53 PM, Linus Walleij wrote:
> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
> <martinez.javier@gmail.com> wrote:
> 
>> Is the following inlined patch [1] what you were thinking that would
>> be the right approach?
> 
> This looks sort of OK, but I'm still struggling with the question of
> what we could do to help other implementations facing the same issue.
> 
> This is a pretty hard design pattern to properly replicate in every such
> driver is it not?

Well, instead of adding .request_irq() to the irqchip, and then making
each driver call gpio_request() from the implementation, perhaps you
could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
and if it gets back a non-error response, the IRQ core could call
gpio_request(). That means that the change to each GPIO+IRQ driver is
simply to implement a standalone data transformation function
.irq_to_gpio().

Now, this does re-introduce irq_to_gpio() in some way, but with the
following advantages:

1) The implementation is per-controller, not a single global function,
so isn't introducing any kind of centralized mapping scheme again.

2) This irq-chip-specific .irq_to_gpio() would only be implemented for
IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
Its potential existence doesn't imply that all IRQ chips need implement
this; it would be very specifically be for this one particular case.

So, I think it's reasonable to introduce this.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/15/2013 11:53 AM, Stephen Warren wrote:
> On 04/13/2013 07:35 PM, Javier Martinez Canillas wrote:
> ...
>> Is the following inlined patch [1] what you were thinking that would
>> be the right approach?
> ...
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> ...
>> +static int omap_gpio_irq_request(struct irq_data *d)
>> +{
>> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> +
>> +	return omap_gpio_request(&bank->chip, d->hwirq);
> 
> If you want the GPIO usage to be known to the GPIO subsystem, then
> wouldn't you call gpio_request() here rather than omap_gpio_request()?
> The above code will certainly do enough so that the OMAP GPIO HW is
> fully enabled as you need, but I thought the idea was to also prevent
> some other code successfully running gpio_request() on that same GPIO?

Also, although omap gpios default to being inputs, we should not assume
that. So may be you should call gpio_request_one() here passing as flags
GPIOF_IN to configure as an input.

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/15/2013 11:58 AM, Stephen Warren wrote:
> On 04/14/2013 02:53 PM, Linus Walleij wrote:
>> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
>> <martinez.javier@gmail.com> wrote:
>>
>>> Is the following inlined patch [1] what you were thinking that would
>>> be the right approach?
>>
>> This looks sort of OK, but I'm still struggling with the question of
>> what we could do to help other implementations facing the same issue.
>>
>> This is a pretty hard design pattern to properly replicate in every such
>> driver is it not?
> 
> Well, instead of adding .request_irq() to the irqchip, and then making
> each driver call gpio_request() from the implementation, perhaps you
> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
> and if it gets back a non-error response, the IRQ core could call
> gpio_request(). That means that the change to each GPIO+IRQ driver is
> simply to implement a standalone data transformation function
> .irq_to_gpio().

I am still concerned about the case where a driver may have already
called gpio_request() and then calls request_irq(). I think that the
solution needs to handle cases where the driver may or may not call
gpio_request() to allocate the gpio.

Although it could be argued that this is problem is not DT specific,
it does become a bigger problem to handle in the case of DT. Therefore,
I am wondering if we should just focus on the DT case for now. 

> Now, this does re-introduce irq_to_gpio() in some way, but with the
> following advantages:
> 
> 1) The implementation is per-controller, not a single global function,
> so isn't introducing any kind of centralized mapping scheme again.
> 
> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for
> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
> Its potential existence doesn't imply that all IRQ chips need implement
> this; it would be very specifically be for this one particular case.
> 
> So, I think it's reasonable to introduce this.

How about using the gpio irq domain xlate function?

Typically, in DT land a device using a gpio as an interrupt source
will have something like the following ...

eth@0 {
	compatible = "ks8851";
	...
	interrupt-parent = <&gpio2>;
	interrupts = <2 8>; /* gpio line 34, low triggered */
};

... or ...

mmc {
	label = "pandaboard::status2";
 	gpios = <&gpio1 8 0>;
	...
};

Both these devices are using a gpio as an interrupt source, but the mmc
driver is requesting the gpio directly. In the first case the xlate
function for the gpio irq domain will be called where as it is not used
in the 2nd case. Therefore, we could add a custom xlate function. For
example ...

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 53bb8d5..caaeab2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1085,6 +1085,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
        irq_set_handler_data(bank->irq, bank);
 }
 
+
+int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
+                         const u32 *intspec, unsigned int intsize,
+                         unsigned long *out_hwirq, unsigned int *out_type)
+{
+       struct gpio_bank *bank;
+       int irq;
+
+       if (WARN_ON(intsize < 1))
+               return -EINVAL;
+
+       irq = irq_find_mapping(d, intspec[0]);
+       bank = irq_get_chip_data(irq);
+       if (!bank)
+               return -ENODEV;
+
+       gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name);
+
+       *out_hwirq = intspec[0];
+       *out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
+       return 0;
+}
+
+static const struct irq_domain_ops omap_irq_domain_ops = {
+       .xlate = omap_irq_domain_xlate,
+};
+
 static const struct of_device_id omap_gpio_match[];
 static void omap_gpio_setup_context(struct gpio_bank *p);
 
@@ -1135,7 +1162,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 
 
        bank->domain = irq_domain_add_linear(node, bank->width,
-                                            &irq_domain_simple_ops, NULL);
+                                            &omap_irq_domain_ops, NULL);
        if (!bank->domain)
                return -ENODEV;
 
Cheers
Jon
 

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/15/2013 04:40 PM, Jon Hunter wrote:
> 
> On 04/15/2013 11:58 AM, Stephen Warren wrote:
>> On 04/14/2013 02:53 PM, Linus Walleij wrote:
>>> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
>>> <martinez.javier@gmail.com> wrote:
>>>
>>>> Is the following inlined patch [1] what you were thinking that would
>>>> be the right approach?
>>>
>>> This looks sort of OK, but I'm still struggling with the question of
>>> what we could do to help other implementations facing the same issue.
>>>
>>> This is a pretty hard design pattern to properly replicate in every such
>>> driver is it not?
>>
>> Well, instead of adding .request_irq() to the irqchip, and then making
>> each driver call gpio_request() from the implementation, perhaps you
>> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
>> and if it gets back a non-error response, the IRQ core could call
>> gpio_request(). That means that the change to each GPIO+IRQ driver is
>> simply to implement a standalone data transformation function
>> .irq_to_gpio().
> 
> I am still concerned about the case where a driver may have already
> called gpio_request() and then calls request_irq(). I think that the
> solution needs to handle cases where the driver may or may not call
> gpio_request() to allocate the gpio.
> 
> Although it could be argued that this is problem is not DT specific,
> it does become a bigger problem to handle in the case of DT. Therefore,
> I am wondering if we should just focus on the DT case for now. 
> 
>> Now, this does re-introduce irq_to_gpio() in some way, but with the
>> following advantages:
>>
>> 1) The implementation is per-controller, not a single global function,
>> so isn't introducing any kind of centralized mapping scheme again.
>>
>> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for
>> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
>> Its potential existence doesn't imply that all IRQ chips need implement
>> this; it would be very specifically be for this one particular case.
>>
>> So, I think it's reasonable to introduce this.
> 
> How about using the gpio irq domain xlate function?
> 
> Typically, in DT land a device using a gpio as an interrupt source
> will have something like the following ...
> 
> eth@0 {
> 	compatible = "ks8851";
> 	...
> 	interrupt-parent = <&gpio2>;
> 	interrupts = <2 8>; /* gpio line 34, low triggered */
> };
> 
> ... or ...
> 
> mmc {
> 	label = "pandaboard::status2";
>  	gpios = <&gpio1 8 0>;
> 	...
> };
> 
> Both these devices are using a gpio as an interrupt source, but the mmc
> driver is requesting the gpio directly. In the first case the xlate
> function for the gpio irq domain will be called where as it is not used
> in the 2nd case. Therefore, we could add a custom xlate function. For
> example ...
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 53bb8d5..caaeab2 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1085,6 +1085,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
>         irq_set_handler_data(bank->irq, bank);
>  }
>  
> +
> +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
> +                         const u32 *intspec, unsigned int intsize,
> +                         unsigned long *out_hwirq, unsigned int *out_type)
> +{
> +       struct gpio_bank *bank;
> +       int irq;
> +
> +       if (WARN_ON(intsize < 1))
> +               return -EINVAL;
> +
> +       irq = irq_find_mapping(d, intspec[0]);
> +       bank = irq_get_chip_data(irq);
> +       if (!bank)
> +               return -ENODEV;
> +
> +       gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name);

By the way, I know that I should check the return code here, but this
was just an example. Also I don't think using ctrlr->name here works
either as this is just "gpio".

Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/15/2013 03:40 PM, Jon Hunter wrote:
> 
> On 04/15/2013 11:58 AM, Stephen Warren wrote:
>> On 04/14/2013 02:53 PM, Linus Walleij wrote:
>>> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
>>> <martinez.javier@gmail.com> wrote:
>>>
>>>> Is the following inlined patch [1] what you were thinking that would
>>>> be the right approach?
>>>
>>> This looks sort of OK, but I'm still struggling with the question of
>>> what we could do to help other implementations facing the same issue.
>>>
>>> This is a pretty hard design pattern to properly replicate in every such
>>> driver is it not?
>>
>> Well, instead of adding .request_irq() to the irqchip, and then making
>> each driver call gpio_request() from the implementation, perhaps you
>> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
>> and if it gets back a non-error response, the IRQ core could call
>> gpio_request(). That means that the change to each GPIO+IRQ driver is
>> simply to implement a standalone data transformation function
>> .irq_to_gpio().
> 
> I am still concerned about the case where a driver may have already
> called gpio_request() and then calls request_irq(). I think that the
> solution needs to handle cases where the driver may or may not call
> gpio_request() to allocate the gpio.

Are there actually drivers that do this? Perhaps they could just be
fixed not to.

> Although it could be argued that this is problem is not DT specific,
> it does become a bigger problem to handle in the case of DT. Therefore,
> I am wondering if we should just focus on the DT case for now. 

That doesn't sound like a good idea; this issue is entirely orthogonal
to DT.

>> Now, this does re-introduce irq_to_gpio() in some way, but with the
>> following advantages:
>>
>> 1) The implementation is per-controller, not a single global function,
>> so isn't introducing any kind of centralized mapping scheme again.
>>
>> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for
>> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
>> Its potential existence doesn't imply that all IRQ chips need implement
>> this; it would be very specifically be for this one particular case.
>>
>> So, I think it's reasonable to introduce this.
> 
> How about using the gpio irq domain xlate function?

That translates DT IRQ-specifiers to Linux IRQ numbers. There's no
reason to believe that, as an absolute rule, it would work for anything
GPIO-related. The fact that in practice most GPIO+IRQ controllers happen
to use the same numbering for GPIOs and IRQs is just co-incidence.

> Typically, in DT land a device using a gpio as an interrupt source
> will have something like the following ...
> 
> eth@0 {
> 	compatible = "ks8851";
> 	...
> 	interrupt-parent = <&gpio2>;
> 	interrupts = <2 8>; /* gpio line 34, low triggered */
> };

OK, that really is an interrupt...

> ... or ...
> 
> mmc {
> 	label = "pandaboard::status2";
>  	gpios = <&gpio1 8 0>;
> 	...
> };

But that's a gpio-leds instance, not an MMC controller... I really
really hope there's no DT node using "gpios" to mean "interrupts"... And
it wouldn't make any sense for an on-SoC device anyway.

> Both these devices are using a gpio as an interrupt source, but the mmc
> driver is requesting the gpio directly. In the first case the xlate
> function for the gpio irq domain will be called where as it is not used
> in the 2nd case. Therefore, we could add a custom xlate function. For
> example ...
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c

> +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
...
> +       gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name);

I guess that could work, but:

a) It still means doing the gpio_request() in each driver instead of
centrally.

b) This approach doesn't solve the issue where some client driver has
already requested the GPIO. This code would simply prevent that call
from succeeding, which would probably make the driver probe() error out,
which isn't any different to the equivalent proposed centralized
gpio_request() inside some request_irq() failing, and causing the
device's probe() to error out.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/15/2013 05:16 PM, Stephen Warren wrote:
> On 04/15/2013 03:40 PM, Jon Hunter wrote:
>>
>> On 04/15/2013 11:58 AM, Stephen Warren wrote:
>>> On 04/14/2013 02:53 PM, Linus Walleij wrote:
>>>> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
>>>> <martinez.javier@gmail.com> wrote:
>>>>
>>>>> Is the following inlined patch [1] what you were thinking that would
>>>>> be the right approach?
>>>>
>>>> This looks sort of OK, but I'm still struggling with the question of
>>>> what we could do to help other implementations facing the same issue.
>>>>
>>>> This is a pretty hard design pattern to properly replicate in every such
>>>> driver is it not?
>>>
>>> Well, instead of adding .request_irq() to the irqchip, and then making
>>> each driver call gpio_request() from the implementation, perhaps you
>>> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
>>> and if it gets back a non-error response, the IRQ core could call
>>> gpio_request(). That means that the change to each GPIO+IRQ driver is
>>> simply to implement a standalone data transformation function
>>> .irq_to_gpio().
>>
>> I am still concerned about the case where a driver may have already
>> called gpio_request() and then calls request_irq(). I think that the
>> solution needs to handle cases where the driver may or may not call
>> gpio_request() to allocate the gpio.
> 
> Are there actually drivers that do this? Perhaps they could just be
> fixed not to.

Yes ideally, but my fear is that there are several. I know both omap
display and mmc drivers do this. There are many drivers calling
gpio_direction_input() but I have not looked to see which of those are
just reading state versus configuring an interrupt.

>> Although it could be argued that this is problem is not DT specific,
>> it does become a bigger problem to handle in the case of DT. Therefore,
>> I am wondering if we should just focus on the DT case for now. 
> 
> That doesn't sound like a good idea; this issue is entirely orthogonal
> to DT.

True, but it is proving to be difficult to find a solution that everyone
can agree on.

>>> Now, this does re-introduce irq_to_gpio() in some way, but with the
>>> following advantages:
>>>
>>> 1) The implementation is per-controller, not a single global function,
>>> so isn't introducing any kind of centralized mapping scheme again.
>>>
>>> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for
>>> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
>>> Its potential existence doesn't imply that all IRQ chips need implement
>>> this; it would be very specifically be for this one particular case.
>>>
>>> So, I think it's reasonable to introduce this.
>>
>> How about using the gpio irq domain xlate function?
> 
> That translates DT IRQ-specifiers to Linux IRQ numbers. There's no
> reason to believe that, as an absolute rule, it would work for anything
> GPIO-related. The fact that in practice most GPIO+IRQ controllers happen
> to use the same numbering for GPIOs and IRQs is just co-incidence.

Yes but provides a hook where we could call gpio_request(). However, I
am not sure if this would be considered abuse :-p

>> Typically, in DT land a device using a gpio as an interrupt source
>> will have something like the following ...
>>
>> eth@0 {
>> 	compatible = "ks8851";
>> 	...
>> 	interrupt-parent = <&gpio2>;
>> 	interrupts = <2 8>; /* gpio line 34, low triggered */
>> };
> 
> OK, that really is an interrupt...
> 
>> ... or ...
>>
>> mmc {
>> 	label = "pandaboard::status2";
>>  	gpios = <&gpio1 8 0>;
>> 	...
>> };
> 
> But that's a gpio-leds instance, not an MMC controller... I really
> really hope there's no DT node using "gpios" to mean "interrupts"... And
> it wouldn't make any sense for an on-SoC device anyway.

Oops yes, I overlooked that. However, the omap mmc driver
(drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and
request_threaded_irq() for the mmc card-detect interrupt. I believe
tegra is doing the same ...

sdhci@78000000 {
	...
	cd-gpios = <&gpio 69 0>; /* gpio PI5 */
	...	
};

>> Both these devices are using a gpio as an interrupt source, but the mmc
>> driver is requesting the gpio directly. In the first case the xlate
>> function for the gpio irq domain will be called where as it is not used
>> in the 2nd case. Therefore, we could add a custom xlate function. For
>> example ...
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> 
>> +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
> ...
>> +       gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name);
> 
> I guess that could work, but:
> 
> a) It still means doing the gpio_request() in each driver instead of
> centrally.

Right this is device specific, but it avoids exposing irq_to_gpio for a
device. However, we could make this generic if we did expose irq_to_gpio
for each device.

> b) This approach doesn't solve the issue where some client driver has
> already requested the GPIO. This code would simply prevent that call
> from succeeding, which would probably make the driver probe() error out,
> which isn't any different to the equivalent proposed centralized
> gpio_request() inside some request_irq() failing, and causing the
> device's probe() to error out.

If some driver is calling gpio_request() directly, then they will most
likely just call gpio_to_irq() when requesting the interrupt and so the
xlate function would not be called in this case (mmc drivers are a good
example). So I don't see that as being a problem. In fact that's the
benefit of this approach as AFAICT it solves this problem.

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/15/2013 05:04 PM, Jon Hunter wrote:
> 
> On 04/15/2013 05:16 PM, Stephen Warren wrote:
>> On 04/15/2013 03:40 PM, Jon Hunter wrote:
...
>>> mmc {
>>> 	label = "pandaboard::status2";
>>>  	gpios = <&gpio1 8 0>;
>>> 	...
>>> };
>>
>> But that's a gpio-leds instance, not an MMC controller... I really
>> really hope there's no DT node using "gpios" to mean "interrupts"... And
>> it wouldn't make any sense for an on-SoC device anyway.
> 
> Oops yes, I overlooked that. However, the omap mmc driver
> (drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and
> request_threaded_irq() for the mmc card-detect interrupt. I believe
> tegra is doing the same ...
> 
> sdhci@78000000 {
> 	...
> 	cd-gpios = <&gpio 69 0>; /* gpio PI5 */
> 	...	
> };

Ah true. I guess at least all MMC drivers are likely hooking cd-gpios as
an interrupt, /and/ requesting it as a GPIO so that they can read the
current state when the GPIO goes off.

That tends to imply that no core code can possibly call gpio_request()
in response to request_irq(), since doing so likely will conflict with
quite a few drivers...

>>> Both these devices are using a gpio as an interrupt source, but the mmc
>>> driver is requesting the gpio directly. In the first case the xlate
>>> function for the gpio irq domain will be called where as it is not used
>>> in the 2nd case. Therefore, we could add a custom xlate function. For
>>> example ...
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>
>>> +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
>> ...
>>> +       gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name);
>>
>> I guess that could work, but:
>>
>> a) It still means doing the gpio_request() in each driver instead of
>> centrally.
> 
> Right this is device specific, but it avoids exposing irq_to_gpio for a
> device. However, we could make this generic if we did expose irq_to_gpio
> for each device.
> 
>> b) This approach doesn't solve the issue where some client driver has
>> already requested the GPIO. This code would simply prevent that call
>> from succeeding, which would probably make the driver probe() error out,
>> which isn't any different to the equivalent proposed centralized
>> gpio_request() inside some request_irq() failing, and causing the
>> device's probe() to error out.
> 
> If some driver is calling gpio_request() directly, then they will most
> likely just call gpio_to_irq() when requesting the interrupt and so the
> xlate function would not be called in this case (mmc drivers are a good
> example). So I don't see that as being a problem. In fact that's the
> benefit of this approach as AFAICT it solves this problem.

Oh. That assumption seems very fragile. What about drivers that actually
do have platform data (or DT bindings) that provide both the IRQ and
GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.

Given all this, I guess simply having each GPIO+IRQ driver's set_type
function simply do whatever is required in HW to set up that GPIO
actually does seem like the best idea. Admittedly that isn't
centralized, but I'm not sure now that any centralized implementation is
possible, without significant rework of a bunch of drivers. This is what
the Tegra GPIO driver already does, and I think one of the earlier
patches in this thread did exactly that for OMAP IRQs too right?

BTW, just so you know, I'm on vacation for 2 weeks starting Wed
afternoon, so replies will be non-existent or spotty during that time.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/16/2013 01:40 PM, Stephen Warren wrote:
> On 04/15/2013 05:04 PM, Jon Hunter wrote:
>>
>> On 04/15/2013 05:16 PM, Stephen Warren wrote:
>>> On 04/15/2013 03:40 PM, Jon Hunter wrote:
> ...
>>>> mmc {
>>>> 	label = "pandaboard::status2";
>>>>  	gpios = <&gpio1 8 0>;
>>>> 	...
>>>> };
>>>
>>> But that's a gpio-leds instance, not an MMC controller... I really
>>> really hope there's no DT node using "gpios" to mean "interrupts"... And
>>> it wouldn't make any sense for an on-SoC device anyway.
>>
>> Oops yes, I overlooked that. However, the omap mmc driver
>> (drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and
>> request_threaded_irq() for the mmc card-detect interrupt. I believe
>> tegra is doing the same ...
>>
>> sdhci@78000000 {
>> 	...
>> 	cd-gpios = <&gpio 69 0>; /* gpio PI5 */
>> 	...	
>> };
> 
> Ah true. I guess at least all MMC drivers are likely hooking cd-gpios as
> an interrupt, /and/ requesting it as a GPIO so that they can read the
> current state when the GPIO goes off.
> 
> That tends to imply that no core code can possibly call gpio_request()
> in response to request_irq(), since doing so likely will conflict with
> quite a few drivers...

Yes that was my concern.

>>>> Both these devices are using a gpio as an interrupt source, but the mmc
>>>> driver is requesting the gpio directly. In the first case the xlate
>>>> function for the gpio irq domain will be called where as it is not used
>>>> in the 2nd case. Therefore, we could add a custom xlate function. For
>>>> example ...
>>>>
>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>
>>>> +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
>>> ...
>>>> +       gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name);
>>>
>>> I guess that could work, but:
>>>
>>> a) It still means doing the gpio_request() in each driver instead of
>>> centrally.
>>
>> Right this is device specific, but it avoids exposing irq_to_gpio for a
>> device. However, we could make this generic if we did expose irq_to_gpio
>> for each device.
>>
>>> b) This approach doesn't solve the issue where some client driver has
>>> already requested the GPIO. This code would simply prevent that call
>>> from succeeding, which would probably make the driver probe() error out,
>>> which isn't any different to the equivalent proposed centralized
>>> gpio_request() inside some request_irq() failing, and causing the
>>> device's probe() to error out.
>>
>> If some driver is calling gpio_request() directly, then they will most
>> likely just call gpio_to_irq() when requesting the interrupt and so the
>> xlate function would not be called in this case (mmc drivers are a good
>> example). So I don't see that as being a problem. In fact that's the
>> benefit of this approach as AFAICT it solves this problem.
> 
> Oh. That assumption seems very fragile. What about drivers that actually
> do have platform data (or DT bindings) that provide both the IRQ and
> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.

Right. In the DT case though, if someone does provide the IRQ and GPIO
IDs then at least they would use a different xlate function. Another
option to consider would be defining the #interrupt-cells = <3> where we
would have ...

cell-#1 --> IRQ domain ID
cell-#2 --> Trigger type
cell-#3 --> GPIO ID

Then we could have a generic xlate for 3 cells that would also request
the GPIO. Again not sure if people are against a gpio being requested in
the xlate but just an idea. Or given that irq_of_parse_and_map() calls
the xlate, we could have this function call gpio_request() if the
interrupt controller is a gpio and there are 3 cells.

> Given all this, I guess simply having each GPIO+IRQ driver's set_type
> function simply do whatever is required in HW to set up that GPIO
> actually does seem like the best idea. Admittedly that isn't
> centralized, but I'm not sure now that any centralized implementation is
> possible, without significant rework of a bunch of drivers. This is what
> the Tegra GPIO driver already does, and I think one of the earlier
> patches in this thread did exactly that for OMAP IRQs too right?

Yes, however, Linus wanted us to make sure the gpio is requested which
is why we have not taken that patch. However, if we cannot find a better
alternative may be we have to do that for now.

> BTW, just so you know, I'm on vacation for 2 weeks starting Wed
> afternoon, so replies will be non-existent or spotty during that time.

Thanks!
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/16/2013 02:27 PM, Jon Hunter wrote:

...

> Right. In the DT case though, if someone does provide the IRQ and GPIO
> IDs then at least they would use a different xlate function. Another
> option to consider would be defining the #interrupt-cells = <3> where we
> would have ...
> 
> cell-#1 --> IRQ domain ID
> cell-#2 --> Trigger type
> cell-#3 --> GPIO ID
> 
> Then we could have a generic xlate for 3 cells that would also request
> the GPIO. Again not sure if people are against a gpio being requested in
> the xlate but just an idea. Or given that irq_of_parse_and_map() calls
> the xlate, we could have this function call gpio_request() if the
> interrupt controller is a gpio and there are 3 cells.

So basically some like the following (note I have only included the omap
portion in here for an example). The other benefit being no need to 
re-introduce irq_to_gpio.

Jon

>From f01ce047075a922969fcbe904b339fe8d03a997b Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Tue, 16 Apr 2013 09:54:28 -0500
Subject: [PATCH] gpio: add custom xlate for gpio irqs

---
 drivers/gpio/gpio-omap.c  |    3 ++-
 drivers/gpio/gpiolib-of.c |   24 ++++++++++++++++++++++++
 include/linux/of_gpio.h   |    1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index d337318..b3d7d43 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -24,6 +24,7 @@
 #include <linux/pm.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/irqdomain.h>
 #include <linux/gpio.h>
 #include <linux/platform_data/gpio-omap.h>
@@ -1135,7 +1136,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 
 
 	bank->domain = irq_domain_add_linear(node, bank->width,
-					     &irq_domain_simple_ops, NULL);
+					     &of_gpio_irq_domain_ops, NULL);
 	if (!bank->domain)
 		return -ENODEV;
 
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 5150df6..fc01ec1 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -15,6 +15,8 @@
 #include <linux/errno.h>
 #include <linux/module.h>
 #include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/gpio.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -253,3 +255,25 @@ void of_gpiochip_remove(struct gpio_chip *chip)
 	if (chip->of_node)
 		of_node_put(chip->of_node);
 }
+
+int of_gpio_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
+                            const u32 *intspec, unsigned int intsize,
+                            unsigned long *out_hwirq, unsigned int *out_type)
+{
+       int ret;
+
+       if (WARN_ON(intsize < 3))
+               return -EINVAL;
+
+       ret = gpio_request_one(intspec[2], GPIOF_IN, NULL);
+       if (ret)
+               return ret;
+
+       *out_hwirq = intspec[0];
+       *out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
+       return 0;
+}
+
+const struct irq_domain_ops of_gpio_irq_domain_ops = {
+       .xlate = of_gpio_irq_domain_xlate,
+};
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index a83dc6f..ad2b962 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -58,6 +58,7 @@ extern void of_gpiochip_remove(struct gpio_chip *gc);
 extern int of_gpio_simple_xlate(struct gpio_chip *gc,
 				const struct of_phandle_args *gpiospec,
 				u32 *flags);
+extern const struct irq_domain_ops of_gpio_irq_domain_ops;
 
 #else /* CONFIG_OF_GPIO */
 
-- 
1.7.10.4

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/16/2013 01:27 PM, Jon Hunter wrote:
> 
> On 04/16/2013 01:40 PM, Stephen Warren wrote:
>> On 04/15/2013 05:04 PM, Jon Hunter wrote:
...
>>> If some driver is calling gpio_request() directly, then they will most
>>> likely just call gpio_to_irq() when requesting the interrupt and so the
>>> xlate function would not be called in this case (mmc drivers are a good
>>> example). So I don't see that as being a problem. In fact that's the
>>> benefit of this approach as AFAICT it solves this problem.
>>
>> Oh. That assumption seems very fragile. What about drivers that actually
>> do have platform data (or DT bindings) that provide both the IRQ and
>> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
> 
> Right. In the DT case though, if someone does provide the IRQ and GPIO
> IDs then at least they would use a different xlate function. Another
> option to consider would be defining the #interrupt-cells = <3> where we
> would have ...
> 
> cell-#1 --> IRQ domain ID
> cell-#2 --> Trigger type
> cell-#3 --> GPIO ID
> 
> Then we could have a generic xlate for 3 cells that would also request
> the GPIO. Again not sure if people are against a gpio being requested in
> the xlate but just an idea. Or given that irq_of_parse_and_map() calls
> the xlate, we could have this function call gpio_request() if the
> interrupt controller is a gpio and there are 3 cells.

I rather dislike this approach since:

a) It requires changes to the DT bindings, which are already defined.
Admittedly it's backwards-compatible, but still.

b) There isn't really any need for the DT to represent this; the
GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
vice-versa (if the HW has such a concept), so there's no need for the DT
to contain this information. This seems like pushing Linux's internal
requirements into the design of the DT binding.

c) I have the feeling that hooking the of_xlate function for this is a
bit of an abuse of the function.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/16/2013 05:11 PM, Stephen Warren wrote:
> On 04/16/2013 01:27 PM, Jon Hunter wrote:
>>
>> On 04/16/2013 01:40 PM, Stephen Warren wrote:
>>> On 04/15/2013 05:04 PM, Jon Hunter wrote:
> ...
>>>> If some driver is calling gpio_request() directly, then they will most
>>>> likely just call gpio_to_irq() when requesting the interrupt and so the
>>>> xlate function would not be called in this case (mmc drivers are a good
>>>> example). So I don't see that as being a problem. In fact that's the
>>>> benefit of this approach as AFAICT it solves this problem.
>>>
>>> Oh. That assumption seems very fragile. What about drivers that actually
>>> do have platform data (or DT bindings) that provide both the IRQ and
>>> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
>>
>> Right. In the DT case though, if someone does provide the IRQ and GPIO
>> IDs then at least they would use a different xlate function. Another
>> option to consider would be defining the #interrupt-cells = <3> where we
>> would have ...
>>
>> cell-#1 --> IRQ domain ID
>> cell-#2 --> Trigger type
>> cell-#3 --> GPIO ID
>>
>> Then we could have a generic xlate for 3 cells that would also request
>> the GPIO. Again not sure if people are against a gpio being requested in
>> the xlate but just an idea. Or given that irq_of_parse_and_map() calls
>> the xlate, we could have this function call gpio_request() if the
>> interrupt controller is a gpio and there are 3 cells.
> 
> I rather dislike this approach since:
>
> a) It requires changes to the DT bindings, which are already defined.
> Admittedly it's backwards-compatible, but still.
> 
> b) There isn't really any need for the DT to represent this; the
> GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
> vice-versa (if the HW has such a concept), so there's no need for the DT
> to contain this information. This seems like pushing Linux's internal
> requirements into the design of the DT binding.

Yes, so the only alternative is to use irq_to_gpio to avoid this.

> c) I have the feeling that hooking the of_xlate function for this is a
> bit of an abuse of the function.

I was wondering about that. So I was grep'ing through the various xlate
implementations and found this [1]. Also you may recall that in the
of_dma_simple_xlate() we call the dma_request_channel() to allocate the
channel, which is very similar. However, I don't wish to get a
reputation as abusing APIs so would be good to know if this really is
abuse or not ;-)

Cheers
Jon

[1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Wed, Apr 17, 2013 at 1:14 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 04/16/2013 05:11 PM, Stephen Warren wrote:
>> On 04/16/2013 01:27 PM, Jon Hunter wrote:
>>>
>>> On 04/16/2013 01:40 PM, Stephen Warren wrote:
>>>> On 04/15/2013 05:04 PM, Jon Hunter wrote:
>> ...
>>>>> If some driver is calling gpio_request() directly, then they will most
>>>>> likely just call gpio_to_irq() when requesting the interrupt and so the
>>>>> xlate function would not be called in this case (mmc drivers are a good
>>>>> example). So I don't see that as being a problem. In fact that's the
>>>>> benefit of this approach as AFAICT it solves this problem.
>>>>
>>>> Oh. That assumption seems very fragile. What about drivers that actually
>>>> do have platform data (or DT bindings) that provide both the IRQ and
>>>> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
>>>
>>> Right. In the DT case though, if someone does provide the IRQ and GPIO
>>> IDs then at least they would use a different xlate function. Another
>>> option to consider would be defining the #interrupt-cells = <3> where we
>>> would have ...
>>>
>>> cell-#1 --> IRQ domain ID
>>> cell-#2 --> Trigger type
>>> cell-#3 --> GPIO ID
>>>
>>> Then we could have a generic xlate for 3 cells that would also request
>>> the GPIO. Again not sure if people are against a gpio being requested in
>>> the xlate but just an idea. Or given that irq_of_parse_and_map() calls
>>> the xlate, we could have this function call gpio_request() if the
>>> interrupt controller is a gpio and there are 3 cells.
>>
>> I rather dislike this approach since:
>>
>> a) It requires changes to the DT bindings, which are already defined.
>> Admittedly it's backwards-compatible, but still.
>>
>> b) There isn't really any need for the DT to represent this; the
>> GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
>> vice-versa (if the HW has such a concept), so there's no need for the DT
>> to contain this information. This seems like pushing Linux's internal
>> requirements into the design of the DT binding.
>
> Yes, so the only alternative is to use irq_to_gpio to avoid this.
>
>> c) I have the feeling that hooking the of_xlate function for this is a
>> bit of an abuse of the function.
>
> I was wondering about that. So I was grep'ing through the various xlate
> implementations and found this [1]. Also you may recall that in the
> of_dma_simple_xlate() we call the dma_request_channel() to allocate the
> channel, which is very similar. However, I don't wish to get a
> reputation as abusing APIs so would be good to know if this really is
> abuse or not ;-)
>
> Cheers
> Jon
>
> [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124

I was looking at [1] shared by Jon and come up with the following
patch that does something similar for OMAP GPIO. This has the
advantage that is local to gpio-omap instead changing gpiolib-of and
also doesn't require DT changes

But I don't want to get a reputation for abusing APIs neither :-)

Best regards,
Javier

>From 23368eb72b125227fcf4b22be19ea70b4ab94556 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Date: Wed, 17 Apr 2013 02:03:08 +0200
Subject: [PATCH 1/1] gpio/omap: add custom xlate function handler

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/gpio/gpio-omap.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 8524ce5..77216f9 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1097,6 +1097,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 static const struct of_device_id omap_gpio_match[];
 static void omap_gpio_init_context(struct gpio_bank *p);

+static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
+				      struct device_node *ctrlr,
+				      const u32 *intspec, unsigned int intsize,
+				      irq_hw_number_t *out_hwirq,
+				      unsigned int *out_type)
+{
+	int ret;
+	struct gpio_bank *bank = d->host_data;
+	int gpio = bank->chip.base + intspec[0];
+
+	if (WARN_ON(intsize < 2))
+		return -EINVAL;
+
+	ret = gpio_request_one(gpio, GPIOF_IN, ctrlr->full_name);
+	if (ret)
+		return ret;
+
+	*out_hwirq = intspec[0];
+	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
+
+	return 0;
+}
+
+static struct irq_domain_ops omap_gpio_irq_ops = {
+	.xlate	= omap_gpio_irq_domain_xlate,
+};
+
 static int omap_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1144,7 +1171,7 @@ static int omap_gpio_probe(struct platform_device *pdev)


 	bank->domain = irq_domain_add_linear(node, bank->width,
-					     &irq_domain_simple_ops, NULL);
+					     &omap_gpio_irq_ops, bank);
 	if (!bank->domain)
 		return -ENODEV;

-- 
1.7.7.6

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/16/2013 07:41 PM, Javier Martinez Canillas wrote:
> On Wed, Apr 17, 2013 at 1:14 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>>
>> On 04/16/2013 05:11 PM, Stephen Warren wrote:
>>> On 04/16/2013 01:27 PM, Jon Hunter wrote:
>>>>
>>>> On 04/16/2013 01:40 PM, Stephen Warren wrote:
>>>>> On 04/15/2013 05:04 PM, Jon Hunter wrote:
>>> ...
>>>>>> If some driver is calling gpio_request() directly, then they will most
>>>>>> likely just call gpio_to_irq() when requesting the interrupt and so the
>>>>>> xlate function would not be called in this case (mmc drivers are a good
>>>>>> example). So I don't see that as being a problem. In fact that's the
>>>>>> benefit of this approach as AFAICT it solves this problem.
>>>>>
>>>>> Oh. That assumption seems very fragile. What about drivers that actually
>>>>> do have platform data (or DT bindings) that provide both the IRQ and
>>>>> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
>>>>
>>>> Right. In the DT case though, if someone does provide the IRQ and GPIO
>>>> IDs then at least they would use a different xlate function. Another
>>>> option to consider would be defining the #interrupt-cells = <3> where we
>>>> would have ...
>>>>
>>>> cell-#1 --> IRQ domain ID
>>>> cell-#2 --> Trigger type
>>>> cell-#3 --> GPIO ID
>>>>
>>>> Then we could have a generic xlate for 3 cells that would also request
>>>> the GPIO. Again not sure if people are against a gpio being requested in
>>>> the xlate but just an idea. Or given that irq_of_parse_and_map() calls
>>>> the xlate, we could have this function call gpio_request() if the
>>>> interrupt controller is a gpio and there are 3 cells.
>>>
>>> I rather dislike this approach since:
>>>
>>> a) It requires changes to the DT bindings, which are already defined.
>>> Admittedly it's backwards-compatible, but still.
>>>
>>> b) There isn't really any need for the DT to represent this; the
>>> GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
>>> vice-versa (if the HW has such a concept), so there's no need for the DT
>>> to contain this information. This seems like pushing Linux's internal
>>> requirements into the design of the DT binding.
>>
>> Yes, so the only alternative is to use irq_to_gpio to avoid this.
>>
>>> c) I have the feeling that hooking the of_xlate function for this is a
>>> bit of an abuse of the function.
>>
>> I was wondering about that. So I was grep'ing through the various xlate
>> implementations and found this [1]. Also you may recall that in the
>> of_dma_simple_xlate() we call the dma_request_channel() to allocate the
>> channel, which is very similar. However, I don't wish to get a
>> reputation as abusing APIs so would be good to know if this really is
>> abuse or not ;-)
>>
>> Cheers
>> Jon
>>
>> [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
> 
> I was looking at [1] shared by Jon and come up with the following
> patch that does something similar for OMAP GPIO. This has the
> advantage that is local to gpio-omap instead changing gpiolib-of and
> also doesn't require DT changes
> 
> But I don't want to get a reputation for abusing APIs neither :-)
> 
> Best regards,
> Javier
> 
> From 23368eb72b125227fcf4b22be19ea70b4ab94556 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Date: Wed, 17 Apr 2013 02:03:08 +0200
> Subject: [PATCH 1/1] gpio/omap: add custom xlate function handler
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/gpio/gpio-omap.c |   29 ++++++++++++++++++++++++++++-
>  1 files changed, 28 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 8524ce5..77216f9 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1097,6 +1097,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
>  static const struct of_device_id omap_gpio_match[];
>  static void omap_gpio_init_context(struct gpio_bank *p);
> 
> +static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
> +				      struct device_node *ctrlr,
> +				      const u32 *intspec, unsigned int intsize,
> +				      irq_hw_number_t *out_hwirq,
> +				      unsigned int *out_type)
> +{
> +	int ret;
> +	struct gpio_bank *bank = d->host_data;
> +	int gpio = bank->chip.base + intspec[0];
> +
> +	if (WARN_ON(intsize < 2))
> +		return -EINVAL;
> +
> +	ret = gpio_request_one(gpio, GPIOF_IN, ctrlr->full_name);
> +	if (ret)
> +		return ret;
> +
> +	*out_hwirq = intspec[0];
> +	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
> +
> +	return 0;
> +}
> +
> +static struct irq_domain_ops omap_gpio_irq_ops = {
> +	.xlate	= omap_gpio_irq_domain_xlate,
> +};
> +
>  static int omap_gpio_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1144,7 +1171,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
> 
> 
>  	bank->domain = irq_domain_add_linear(node, bank->width,
> -					     &irq_domain_simple_ops, NULL);
> +					     &omap_gpio_irq_ops, bank);
>  	if (!bank->domain)
>  		return -ENODEV;
> 

That would work for omap, in fact I posted pretty much the same thing a
day ago [1] ;-)

I was trying to see if we could find a common solution that everyone
could use as it seems that ideally we should all be requesting the gpio.

Cheers
Jon

[1] http://marc.info/?l=linux-arm-kernel&m=136606204823845&w=1

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Wed, Apr 17, 2013 at 4:00 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 04/16/2013 07:41 PM, Javier Martinez Canillas wrote:
>> On Wed, Apr 17, 2013 at 1:14 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>>>
>>> On 04/16/2013 05:11 PM, Stephen Warren wrote:
>>>> On 04/16/2013 01:27 PM, Jon Hunter wrote:
>>>>>
>>>>> On 04/16/2013 01:40 PM, Stephen Warren wrote:
>>>>>> On 04/15/2013 05:04 PM, Jon Hunter wrote:
>>>> ...
>>>>>>> If some driver is calling gpio_request() directly, then they will most
>>>>>>> likely just call gpio_to_irq() when requesting the interrupt and so the
>>>>>>> xlate function would not be called in this case (mmc drivers are a good
>>>>>>> example). So I don't see that as being a problem. In fact that's the
>>>>>>> benefit of this approach as AFAICT it solves this problem.
>>>>>>
>>>>>> Oh. That assumption seems very fragile. What about drivers that actually
>>>>>> do have platform data (or DT bindings) that provide both the IRQ and
>>>>>> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
>>>>>
>>>>> Right. In the DT case though, if someone does provide the IRQ and GPIO
>>>>> IDs then at least they would use a different xlate function. Another
>>>>> option to consider would be defining the #interrupt-cells = <3> where we
>>>>> would have ...
>>>>>
>>>>> cell-#1 --> IRQ domain ID
>>>>> cell-#2 --> Trigger type
>>>>> cell-#3 --> GPIO ID
>>>>>
>>>>> Then we could have a generic xlate for 3 cells that would also request
>>>>> the GPIO. Again not sure if people are against a gpio being requested in
>>>>> the xlate but just an idea. Or given that irq_of_parse_and_map() calls
>>>>> the xlate, we could have this function call gpio_request() if the
>>>>> interrupt controller is a gpio and there are 3 cells.
>>>>
>>>> I rather dislike this approach since:
>>>>
>>>> a) It requires changes to the DT bindings, which are already defined.
>>>> Admittedly it's backwards-compatible, but still.
>>>>
>>>> b) There isn't really any need for the DT to represent this; the
>>>> GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
>>>> vice-versa (if the HW has such a concept), so there's no need for the DT
>>>> to contain this information. This seems like pushing Linux's internal
>>>> requirements into the design of the DT binding.
>>>
>>> Yes, so the only alternative is to use irq_to_gpio to avoid this.
>>>
>>>> c) I have the feeling that hooking the of_xlate function for this is a
>>>> bit of an abuse of the function.
>>>
>>> I was wondering about that. So I was grep'ing through the various xlate
>>> implementations and found this [1]. Also you may recall that in the
>>> of_dma_simple_xlate() we call the dma_request_channel() to allocate the
>>> channel, which is very similar. However, I don't wish to get a
>>> reputation as abusing APIs so would be good to know if this really is
>>> abuse or not ;-)
>>>
>>> Cheers
>>> Jon
>>>
>>> [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
>>
>> I was looking at [1] shared by Jon and come up with the following
>> patch that does something similar for OMAP GPIO. This has the
>> advantage that is local to gpio-omap instead changing gpiolib-of and
>> also doesn't require DT changes
>>
>> But I don't want to get a reputation for abusing APIs neither :-)
>>
>> Best regards,
>> Javier
>>
>> From 23368eb72b125227fcf4b22be19ea70b4ab94556 Mon Sep 17 00:00:00 2001
>> From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> Date: Wed, 17 Apr 2013 02:03:08 +0200
>> Subject: [PATCH 1/1] gpio/omap: add custom xlate function handler
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>>  drivers/gpio/gpio-omap.c |   29 ++++++++++++++++++++++++++++-
>>  1 files changed, 28 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 8524ce5..77216f9 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -1097,6 +1097,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
>>  static const struct of_device_id omap_gpio_match[];
>>  static void omap_gpio_init_context(struct gpio_bank *p);
>>
>> +static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
>> +                                   struct device_node *ctrlr,
>> +                                   const u32 *intspec, unsigned int intsize,
>> +                                   irq_hw_number_t *out_hwirq,
>> +                                   unsigned int *out_type)
>> +{
>> +     int ret;
>> +     struct gpio_bank *bank = d->host_data;
>> +     int gpio = bank->chip.base + intspec[0];
>> +
>> +     if (WARN_ON(intsize < 2))
>> +             return -EINVAL;
>> +
>> +     ret = gpio_request_one(gpio, GPIOF_IN, ctrlr->full_name);
>> +     if (ret)
>> +             return ret;
>> +
>> +     *out_hwirq = intspec[0];
>> +     *out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
>> +
>> +     return 0;
>> +}
>> +
>> +static struct irq_domain_ops omap_gpio_irq_ops = {
>> +     .xlate  = omap_gpio_irq_domain_xlate,
>> +};
>> +
>>  static int omap_gpio_probe(struct platform_device *pdev)
>>  {
>>       struct device *dev = &pdev->dev;
>> @@ -1144,7 +1171,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
>>
>>
>>       bank->domain = irq_domain_add_linear(node, bank->width,
>> -                                          &irq_domain_simple_ops, NULL);
>> +                                          &omap_gpio_irq_ops, bank);
>>       if (!bank->domain)
>>               return -ENODEV;
>>
>
> That would work for omap, in fact I posted pretty much the same thing a
> day ago [1] ;-)
>

Hi Jon,

There are so many patches flying around in this thread that I missed it :-)

Sorry about that...

> I was trying to see if we could find a common solution that everyone
> could use as it seems that ideally we should all be requesting the gpio.
>
> Cheers
> Jon
>
> [1] http://marc.info/?l=linux-arm-kernel&m=136606204823845&w=1

btw, I shared the latest patch with only build testing it, but today I
gave a try and I found a problem with this approach. The .xlate
function is being called twice for each GPIO-IRQ so the first time
gpio_request_one() succeeds but the second time it fails returning
-EBUSY.

This raises a warning on drivers/of/platform.c
(WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq)):

    0.285308] ------------[ cut here ]------------
[    0.285369] WARNING: at drivers/of/platform.c:171
of_device_alloc+0x154/0x168()
[    0.285430] Modules linked in:
[    0.285491] [<c001a944>] (unwind_backtrace+0x0/0xf0) from
[<c0041edc>] (warn_slowpath_common+0x4c/0x68)
[    0.285552] [<c0041edc>] (warn_slowpath_common+0x4c/0x68) from
[<c0041f14>] (warn_slowpath_null+0x1c/0x24)
[    0.285614] [<c0041f14>] (warn_slowpath_null+0x1c/0x24) from
[<c041ac3c>] (of_device_alloc+0x154/0x168)
[    0.285675] [<c041ac3c>] (of_device_alloc+0x154/0x168) from
[<c041ac84>] (of_platform_device_create_pdata+0x34/0x80)
[    0.285736] [<c041ac84>]
(of_platform_device_create_pdata+0x34/0x80) from [<c0027364>]
(gpmc_probe_generic_child+0x180/0x240)
[    0.285827] [<c0027364>] (gpmc_probe_generic_child+0x180/0x240)
from [<c00278d8>] (gpmc_probe+0x4b4/0x614)
[    0.285888] [<c00278d8>] (gpmc_probe+0x4b4/0x614) from [<c0325514>]
(platform_drv_probe+0x18/0x1c)
[    0.285949] [<c0325514>] (platform_drv_probe+0x18/0x1c) from
[<c0324354>] (driver_probe_device+0x108/0x21c)
[    0.286010] [<c0324354>] (driver_probe_device+0x108/0x21c) from
[<c0322b2c>] (bus_for_each_drv+0x5c/0x88)
[    0.286071] [<c0322b2c>] (bus_for_each_drv+0x5c/0x88) from
[<c0324218>] (device_attach+0x78/0x90)
[    0.286132] [<c0324218>] (device_attach+0x78/0x90) from
[<c0323868>] (bus_probe_device+0x88/0xac)
[    0.286193] [<c0323868>] (bus_probe_device+0x88/0xac) from
[<c03220e8>] (device_add+0x4b0/0x584)
[    0.286254] [<c03220e8>] (device_add+0x4b0/0x584) from [<c041acac>]
(of_platform_device_create_pdata+0x5c/0x80)
[    0.286315] [<c041acac>]
(of_platform_device_create_pdata+0x5c/0x80) from [<c041ad9c>]
(of_platform_bus_create+0xcc/0x278)
[    0.286376] [<c041ad9c>] (of_platform_bus_create+0xcc/0x278) from
[<c041adfc>] (of_platform_bus_create+0x12c/0x278)
[    0.286468] [<c041adfc>] (of_platform_bus_create+0x12c/0x278) from
[<c041afa8>] (of_platform_populate+0x60/0x98)
[    0.286529] [<c041afa8>] (of_platform_populate+0x60/0x98) from
[<c070c5f8>] (omap_generic_init+0x24/0x60)
[    0.286590] [<c070c5f8>] (omap_generic_init+0x24/0x60) from
[<c06fe4c4>] (customize_machine+0x1c/0x28)
[    0.286651] [<c06fe4c4>] (customize_machine+0x1c/0x28) from
[<c00086a4>] (do_one_initcall+0x34/0x178)
[    0.286712] [<c00086a4>] (do_one_initcall+0x34/0x178) from
[<c06fb8f4>] (kernel_init_freeable+0xfc/0x1c8)
[    0.286804] [<c06fb8f4>] (kernel_init_freeable+0xfc/0x1c8) from
[<c04e4668>] (kernel_init+0x8/0xe4)
[    0.286865] [<c04e4668>] (kernel_init+0x8/0xe4) from [<c0013170>]
(ret_from_fork+0x14/0x24)
[    0.287139] ---[ end trace d49d2507892be205 ]---

I probably won't have time to dig further on this until later this
week but I wanted to share with you in case you know why is being
calling twice and if you thought about a solution.

It works if I don't check the return gpio_request_one() (or better if
we don't return on omap_gpio_irq_domain_xlate) but of course is not
the right solution.

Thanks a lot and best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote:

...

> There are so many patches flying around in this thread that I missed it :-)
> 
> Sorry about that...

No problem.

>> I was trying to see if we could find a common solution that everyone
>> could use as it seems that ideally we should all be requesting the gpio.
>>
>> Cheers
>> Jon
>>
>> [1] http://marc.info/?l=linux-arm-kernel&m=136606204823845&w=1
> 
> btw, I shared the latest patch with only build testing it, but today I
> gave a try and I found a problem with this approach. The .xlate
> function is being called twice for each GPIO-IRQ so the first time
> gpio_request_one() succeeds but the second time it fails returning
> -EBUSY.

I tried it and I did not see that. I don't see the below warning either.

> This raises a warning on drivers/of/platform.c
> (WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq)):
> 
>     0.285308] ------------[ cut here ]------------
> [    0.285369] WARNING: at drivers/of/platform.c:171
> of_device_alloc+0x154/0x168()
> [    0.285430] Modules linked in:
> [    0.285491] [<c001a944>] (unwind_backtrace+0x0/0xf0) from
> [<c0041edc>] (warn_slowpath_common+0x4c/0x68)
> [    0.285552] [<c0041edc>] (warn_slowpath_common+0x4c/0x68) from
> [<c0041f14>] (warn_slowpath_null+0x1c/0x24)
> [    0.285614] [<c0041f14>] (warn_slowpath_null+0x1c/0x24) from
> [<c041ac3c>] (of_device_alloc+0x154/0x168)
> [    0.285675] [<c041ac3c>] (of_device_alloc+0x154/0x168) from
> [<c041ac84>] (of_platform_device_create_pdata+0x34/0x80)
> [    0.285736] [<c041ac84>]
> (of_platform_device_create_pdata+0x34/0x80) from [<c0027364>]
> (gpmc_probe_generic_child+0x180/0x240)
> [    0.285827] [<c0027364>] (gpmc_probe_generic_child+0x180/0x240)
> from [<c00278d8>] (gpmc_probe+0x4b4/0x614)
> [    0.285888] [<c00278d8>] (gpmc_probe+0x4b4/0x614) from [<c0325514>]
> (platform_drv_probe+0x18/0x1c)
> [    0.285949] [<c0325514>] (platform_drv_probe+0x18/0x1c) from
> [<c0324354>] (driver_probe_device+0x108/0x21c)

Any chance you have still have some additional code in your dts to
request the gpio? I recall you made some hacks to make this work before.

> I probably won't have time to dig further on this until later this
> week but I wanted to share with you in case you know why is being
> calling twice and if you thought about a solution.

Care to post your dts file?

> It works if I don't check the return gpio_request_one() (or better if
> we don't return on omap_gpio_irq_domain_xlate) but of course is not
> the right solution.

Yes we need to check the return value.

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote:
>
> ...
>
>> There are so many patches flying around in this thread that I missed it :-)
>>
>> Sorry about that...
>
> No problem.
>
>>> I was trying to see if we could find a common solution that everyone
>>> could use as it seems that ideally we should all be requesting the gpio.
>>>
>>> Cheers
>>> Jon
>>>
>>> [1] http://marc.info/?l=linux-arm-kernel&m=136606204823845&w=1
>>
>> btw, I shared the latest patch with only build testing it, but today I
>> gave a try and I found a problem with this approach. The .xlate
>> function is being called twice for each GPIO-IRQ so the first time
>> gpio_request_one() succeeds but the second time it fails returning
>> -EBUSY.
>
> I tried it and I did not see that. I don't see the below warning either.
>

weird, I wonder what's different here.

I'll try tonight to test using another branch based on your
omap-daily-testing branch.

>> This raises a warning on drivers/of/platform.c
>> (WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq)):
>>
>>     0.285308] ------------[ cut here ]------------
>> [    0.285369] WARNING: at drivers/of/platform.c:171
>> of_device_alloc+0x154/0x168()
>> [    0.285430] Modules linked in:
>> [    0.285491] [<c001a944>] (unwind_backtrace+0x0/0xf0) from
>> [<c0041edc>] (warn_slowpath_common+0x4c/0x68)
>> [    0.285552] [<c0041edc>] (warn_slowpath_common+0x4c/0x68) from
>> [<c0041f14>] (warn_slowpath_null+0x1c/0x24)
>> [    0.285614] [<c0041f14>] (warn_slowpath_null+0x1c/0x24) from
>> [<c041ac3c>] (of_device_alloc+0x154/0x168)
>> [    0.285675] [<c041ac3c>] (of_device_alloc+0x154/0x168) from
>> [<c041ac84>] (of_platform_device_create_pdata+0x34/0x80)
>> [    0.285736] [<c041ac84>]
>> (of_platform_device_create_pdata+0x34/0x80) from [<c0027364>]
>> (gpmc_probe_generic_child+0x180/0x240)
>> [    0.285827] [<c0027364>] (gpmc_probe_generic_child+0x180/0x240)
>> from [<c00278d8>] (gpmc_probe+0x4b4/0x614)
>> [    0.285888] [<c00278d8>] (gpmc_probe+0x4b4/0x614) from [<c0325514>]
>> (platform_drv_probe+0x18/0x1c)
>> [    0.285949] [<c0325514>] (platform_drv_probe+0x18/0x1c) from
>> [<c0324354>] (driver_probe_device+0x108/0x21c)
>
> Any chance you have still have some additional code in your dts to
> request the gpio? I recall you made some hacks to make this work before.
>

Yes, but I remove all those hacks from my DT and gpmc driver. Is the
first thing I thought and I already doble checked that.

>> I probably won't have time to dig further on this until later this
>> week but I wanted to share with you in case you know why is being
>> calling twice and if you thought about a solution.
>
> Care to post your dts file?
>

I'm using the following patch to add smsc ethernet support to my board
+ adding 'ranges = <5 0 0x2c000000 0x1000000>;' to the gpmc device
node on omap3.dtsi:

>From 4fe26a40229e6e97c2ab3b80865c9f24e8ff3424 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Date: Wed, 27 Feb 2013 02:59:29 +0100
Subject: [PATCH 2/2] ARM: dts: omap3-igep0020: Add SMSC911x LAN chip support

The IGEPv2 board has an SMSC LAN9221i ethernet chip connected to
the OMAP3 processor though the General-Purpose Memory Controller.

This patch adds a device node for the ethernet chip as a GPMC child
and all its requirements (regulators, GPIO and pin muxs).

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/omap3-igep.dtsi    |    6 ++++
 arch/arm/boot/dts/omap3-igep0020.dts |   52 ++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/omap3-igep.dtsi
b/arch/arm/boot/dts/omap3-igep.dtsi
index f8fe3b7..d5cd504 100644
--- a/arch/arm/boot/dts/omap3-igep.dtsi
+++ b/arch/arm/boot/dts/omap3-igep.dtsi
@@ -62,6 +62,12 @@
 			0x126 0x0100	/* sdmmc1_dat7.sdmmc1_dat7 INPUT | MODE 0 */
 		>;
 	};
+
+	smsc911x_pins: pinmux_smsc911x_pins {
+		pinctrl-single,pins = <
+                        0x1a2 0x0104    /* mcspi1_cs2.gpio_176 INPUT | MODE4 */
+		>;
+	};
 };

 &i2c1 {
diff --git a/arch/arm/boot/dts/omap3-igep0020.dts
b/arch/arm/boot/dts/omap3-igep0020.dts
index e2b9849..32a59df 100644
--- a/arch/arm/boot/dts/omap3-igep0020.dts
+++ b/arch/arm/boot/dts/omap3-igep0020.dts
@@ -40,6 +40,18 @@
 			gpios = <&twl_gpio 19 1>;
 		};
 	};
+
+	vddvario: regulator-vddvario {
+		  compatible = "regulator-fixed";
+		  regulator-name = "vddvario";
+		  regulator-always-on;
+	};
+
+	vdd33a: regulator-vdd33a {
+		compatible = "regulator-fixed";
+		regulator-name = "vdd33a";
+		regulator-always-on;
+	};
 };

 &i2c3 {
@@ -54,3 +66,43 @@
 		reg = <0x50>;
 	};
 };
+
+&gpmc {
+	ethernet@5,0 {
+		pinctrl-names = "default";
+		pinctrl-0 = <&smsc911x_pins>;
+		compatible = "smsc,lan9221", "smsc,lan9115";
+		reg = <5 0 0xff>;
+		bank-width = <2>;
+
+		gpmc,mux-add-data;
+		gpmc,cs-on-ns = <0>;
+		gpmc,cs-rd-off-ns = <186>;
+		gpmc,cs-wr-off-ns = <186>;
+		gpmc,adv-on-ns = <12>;
+		gpmc,adv-rd-off-ns = <48>;
+		gpmc,adv-wr-off-ns = <48>;
+		gpmc,oe-on-ns = <54>;
+		gpmc,oe-off-ns = <168>;
+		gpmc,we-on-ns = <54>;
+		gpmc,we-off-ns = <168>;
+		gpmc,rd-cycle-ns = <186>;
+		gpmc,wr-cycle-ns = <186>;
+		gpmc,access-ns = <114>;
+		gpmc,page-burst-access-ns = <6>;
+		gpmc,bus-turnaround-ns = <12>;
+		gpmc,cycle2cycle-delay-ns = <18>;
+		gpmc,wr-data-mux-bus-ns = <90>;
+		gpmc,wr-access-ns = <186>;
+		gpmc,cycle2cycle-samecsen;
+		gpmc,cycle2cycle-diffcsen;
+
+		interrupt-parent = <&gpio6>;
+		interrupts = <16 8>;
+		vmmc-supply = <&vddvario>;
+		vmmc_aux-supply = <&vdd33a>;
+		reg-io-width = <4>;
+
+		smsc,save-mac-address;
+	};
+};
-- 
1.7.7.6

>> It works if I don't check the return gpio_request_one() (or better if
>> we don't return on omap_gpio_irq_domain_xlate) but of course is not
>> the right solution.
>
> Yes we need to check the return value.
>

I know, I was just pointing out that the fact that gpio_request_one()
was failing is what propagates the error to drivers/of/platform.c

> Cheers
> Jon

Thanks a lot and best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/17/2013 08:42 AM, Javier Martinez Canillas wrote:
> On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter <jon-hunter-l0cyMroinI0@public.gmane.org> wrote:
>>
>> On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote:
>>
>> ...
>>
>>> There are so many patches flying around in this thread that I missed it :-)
>>>
>>> Sorry about that...
>>
>> No problem.
>>
>>>> I was trying to see if we could find a common solution that everyone
>>>> could use as it seems that ideally we should all be requesting the gpio.
>>>>
>>>> Cheers
>>>> Jon
>>>>
>>>> [1] http://marc.info/?l=linux-arm-kernel&m=136606204823845&w=1
>>>
>>> btw, I shared the latest patch with only build testing it, but today I
>>> gave a try and I found a problem with this approach. The .xlate
>>> function is being called twice for each GPIO-IRQ so the first time
>>> gpio_request_one() succeeds but the second time it fails returning
>>> -EBUSY.
>>
>> I tried it and I did not see that. I don't see the below warning either.
>>
> 
> weird, I wonder what's different here.

I am testing on an omap4-sdp which uses a spi based ethernet device.
However, I could try with the omap3430-sdp which uses gpmc.

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Wed, Apr 17, 2013 at 3:52 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 04/17/2013 08:42 AM, Javier Martinez Canillas wrote:
>> On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>>>
>>> On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote:
>>>
>>> ...
>>>
>>>> There are so many patches flying around in this thread that I missed it :-)
>>>>
>>>> Sorry about that...
>>>
>>> No problem.
>>>
>>>>> I was trying to see if we could find a common solution that everyone
>>>>> could use as it seems that ideally we should all be requesting the gpio.
>>>>>
>>>>> Cheers
>>>>> Jon
>>>>>
>>>>> [1] http://marc.info/?l=linux-arm-kernel&m=136606204823845&w=1
>>>>
>>>> btw, I shared the latest patch with only build testing it, but today I
>>>> gave a try and I found a problem with this approach. The .xlate
>>>> function is being called twice for each GPIO-IRQ so the first time
>>>> gpio_request_one() succeeds but the second time it fails returning
>>>> -EBUSY.
>>>
>>> I tried it and I did not see that. I don't see the below warning either.
>>>
>>
>> weird, I wonder what's different here.
>
> I am testing on an omap4-sdp which uses a spi based ethernet device.
> However, I could try with the omap3430-sdp which uses gpmc.
>

Thanks, that would be great and it could be a difference.

But don't worry I'll to test it more extensively when I have some free
time. I just shared here in case you had the same issue.

> Cheers
> Jon

Thanks a lot for your help,
Jaiver

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren]

On 04/16/2013 05:14 PM, Jon Hunter wrote:
> 
> On 04/16/2013 05:11 PM, Stephen Warren wrote:
>> On 04/16/2013 01:27 PM, Jon Hunter wrote:
>>>
>>> On 04/16/2013 01:40 PM, Stephen Warren wrote:
>>>> On 04/15/2013 05:04 PM, Jon Hunter wrote:
>> ...
>>>>> If some driver is calling gpio_request() directly, then they will most
>>>>> likely just call gpio_to_irq() when requesting the interrupt and so the
>>>>> xlate function would not be called in this case (mmc drivers are a good
>>>>> example). So I don't see that as being a problem. In fact that's the
>>>>> benefit of this approach as AFAICT it solves this problem.
>>>>
>>>> Oh. That assumption seems very fragile. What about drivers that actually
>>>> do have platform data (or DT bindings) that provide both the IRQ and
>>>> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.
>>>
>>> Right. In the DT case though, if someone does provide the IRQ and GPIO
>>> IDs then at least they would use a different xlate function. Another
>>> option to consider would be defining the #interrupt-cells = <3> where we
>>> would have ...
>>>
>>> cell-#1 --> IRQ domain ID
>>> cell-#2 --> Trigger type
>>> cell-#3 --> GPIO ID
>>>
>>> Then we could have a generic xlate for 3 cells that would also request
>>> the GPIO. Again not sure if people are against a gpio being requested in
>>> the xlate but just an idea. Or given that irq_of_parse_and_map() calls
>>> the xlate, we could have this function call gpio_request() if the
>>> interrupt controller is a gpio and there are 3 cells.
>>
>> I rather dislike this approach since:
>>
>> a) It requires changes to the DT bindings, which are already defined.
>> Admittedly it's backwards-compatible, but still.
>>
>> b) There isn't really any need for the DT to represent this; the
>> GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
>> vice-versa (if the HW has such a concept), so there's no need for the DT
>> to contain this information. This seems like pushing Linux's internal
>> requirements into the design of the DT binding.
> 
> Yes, so the only alternative is to use irq_to_gpio to avoid this.
> 
>> c) I have the feeling that hooking the of_xlate function for this is a
>> bit of an abuse of the function.
> 
> I was wondering about that. So I was grep'ing through the various xlate
> implementations and found this [1]. Also you may recall that in the
> of_dma_simple_xlate() we call the dma_request_channel() to allocate the
> channel, which is very similar. However, I don't wish to get a
> reputation as abusing APIs so would be good to know if this really is
> abuse or not ;-)
> 
> Cheers
> Jon
> 
> [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
> 

Interesting.

This is really something that the core DT and GPIO and IRQ maintainers
should weigh in on. Hence, changing them from Cc: to To: in this message
and/or adding them.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas]

On Wed, Apr 17, 2013 at 3:52 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 04/17/2013 08:42 AM, Javier Martinez Canillas wrote:
>> On Wed, Apr 17, 2013 at 3:25 PM, Jon Hunter <jon-hunter@ti.com> wrote:
>>>
>>> On 04/17/2013 02:55 AM, Javier Martinez Canillas wrote:
>>>
>>> ...
>>>
>>>> There are so many patches flying around in this thread that I missed it :-)
>>>>
>>>> Sorry about that...
>>>
>>> No problem.
>>>
>>>>> I was trying to see if we could find a common solution that everyone
>>>>> could use as it seems that ideally we should all be requesting the gpio.
>>>>>
>>>>> Cheers
>>>>> Jon
>>>>>
>>>>> [1] http://marc.info/?l=linux-arm-kernel&m=136606204823845&w=1
>>>>
>>>> btw, I shared the latest patch with only build testing it, but today I
>>>> gave a try and I found a problem with this approach. The .xlate
>>>> function is being called twice for each GPIO-IRQ so the first time
>>>> gpio_request_one() succeeds but the second time it fails returning
>>>> -EBUSY.
>>>
>>> I tried it and I did not see that. I don't see the below warning either.
>>>
>>
>> weird, I wonder what's different here.
>
> I am testing on an omap4-sdp which uses a spi based ethernet device.
> However, I could try with the omap3430-sdp which uses gpmc.
>
> Cheers
> Jon

Hi Jon,

I just created a new branch using your omap-daily-testing as a base
and cherry-picked all the required patches and the .xlate function is
working correctly. I don't see the WARN anymore neither is called
twice.

Sorry for the noise..

Best regards,
Javier

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Mon, Apr 15, 2013 at 6:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/14/2013 02:53 PM, Linus Walleij wrote:

>> This is a pretty hard design pattern to properly replicate in every such
>> driver is it not?
>
> Well, instead of adding .request_irq() to the irqchip, and then making
> each driver call gpio_request() from the implementation, perhaps you
> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
> and if it gets back a non-error response, the IRQ core could call
> gpio_request(). That means that the change to each GPIO+IRQ driver is
> simply to implement a standalone data transformation function
> .irq_to_gpio().
>
> Now, this does re-introduce irq_to_gpio() in some way, but with the
> following advantages:
>
> 1) The implementation is per-controller, not a single global function,
> so isn't introducing any kind of centralized mapping scheme again.
>
> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for
> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
> Its potential existence doesn't imply that all IRQ chips need implement
> this; it would be very specifically be for this one particular case.

I sort of like the idea. It'd have to go past Grant and Thomas G though.

A problem is that this will result in the same kind of dilemma that
the pinctrl subsystem has been facing with mapping the global GPIO
numbers back into local numbers (as is the nature of IRQ numbers).
Such as is done by the GPIO ranges in pinctrl.

The good part is that we now know how to actually contain that, even
how to do it in DT.

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Mon, Apr 15, 2013 at 11:40 PM, Jon Hunter <jon-hunter@ti.com> wrote:

> I am still concerned about the case where a driver may have already
> called gpio_request() and then calls request_irq(). I think that the
> solution needs to handle cases where the driver may or may not call
> gpio_request() to allocate the gpio.

So take a step back and you see that this is my actual fear and why
I'm so hesitant about this patch to begin with.

I wanted drivers to follow the sequence:
gpio_request(), gpio_to_irq(), request_irq()

Now if request_irq() can implicitly perform gpio_request(), we end
up with a semantic confusing mess like this where we are
uncertain as to whether we should use the irq we obtain elsewhere
(like from a resource on the platform device)) or maybe use
gpio_to_irq() to convert it into an IRQ or not or ...

It is certainly true that for some devices we don't want to know if
the IRQ comes from a hard-wired IRQ line or from a GPIO line
playing the rôle of IRQ controller, so we need to handle that.

But then the drivers need to be consistent, and relying solely on
code review to stop people from doing mixtures like the described
is not working, the subsystem must enforce a proper policy.

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

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Wed, Apr 17, 2013 at 5:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/16/2013 05:14 PM, Jon Hunter wrote:

>>> c) I have the feeling that hooking the of_xlate function for this is a
>>> bit of an abuse of the function.
>>
>> I was wondering about that. So I was grep'ing through the various xlate
>> implementations and found this [1]. Also you may recall that in the
>> of_dma_simple_xlate() we call the dma_request_channel() to allocate the
>> channel, which is very similar. However, I don't wish to get a
>> reputation as abusing APIs so would be good to know if this really is
>> abuse or not ;-)
>>
>> Cheers
>> Jon
>>
>> [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
>
> Interesting.
>
> This is really something that the core DT and GPIO and IRQ maintainers
> should weigh in on. Hence, changing them from Cc: to To: in this message
> and/or adding them.

So I guess it is OK for a driver or DT node to use a GPIO as IRQ
*only*, and then have the GPIO requested implicitly through the
xlate function in the irqdomain.

It's the use cases where one and the same driver tries to use the
same GPIO line *also* by requesting it using gpio_request()
that madness starts. In such cases the driver deserves to have
an error thrown back at it, as it definately knows the GPIO pin
and can be refactored to use gpio_to_irq() to translate it into
an IRQ rather than having it implicitly done in the xlate function.

You will just have to give the xlate function information about which
GPIOs are used as IRQs only, and only request the GPIO on these.

And I recently suggested a mechanism to do that, and that is
what I called GPIO input-hogs, which means that you mark
(e.g. from the device tree) at probe() of the gpiochip which GPIOs
will only be used as inputs, so as to generate IRQs.

This is perfectly OS-neutral information about the how the
hardware is set up in the system.

If you only allow these hogges inputs to be translated and
requested in the xlate function, everything goes smooth.

It all comes back to this: keep all applicable knowledge in the
system, so it can make proper decisions, don't try to create
mechanisms that will half-guess things.

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas
<martinez.javier@gmail.com> wrote:

So:

> +static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
> +                                     struct device_node *ctrlr,
> +                                     const u32 *intspec, unsigned int intsize,
> +                                     irq_hw_number_t *out_hwirq,
> +                                     unsigned int *out_type)
> +{
> +       int ret;
> +       struct gpio_bank *bank = d->host_data;
> +       int gpio = bank->chip.base + intspec[0];
> +
> +       if (WARN_ON(intsize < 2))
> +               return -EINVAL;
> +
> +       ret = gpio_request_one(gpio, GPIOF_IN, ctrlr->full_name);
> +       if (ret)
> +               return ret;

So how to figure out if a device is already requesting this GPIO
on some orthogonal axis?

if (this_gpio_was_hogged_as_input_in_gpiolib) {
   ret = gpio_request_on()...
}

Is my suggestion, then you can explicitly mark which GPIOs
shall have their IRQs implicitly translated to GPIOs and
requested.

Alas, this requires the DTS:es to have this hogging added, but
it is fully backwards-compatible.

Then the applicabl OMAP drivers (like MMC) can be fixed to
do gpio_to_irq() on  their pins instead of using the IRQ
directly.

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/26/2013 02:27 AM, Linus Walleij wrote:
> On Wed, Apr 17, 2013 at 5:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/16/2013 05:14 PM, Jon Hunter wrote:
> 
>>>> c) I have the feeling that hooking the of_xlate function for this is a
>>>> bit of an abuse of the function.
>>>
>>> I was wondering about that. So I was grep'ing through the various xlate
>>> implementations and found this [1]. Also you may recall that in the
>>> of_dma_simple_xlate() we call the dma_request_channel() to allocate the
>>> channel, which is very similar. However, I don't wish to get a
>>> reputation as abusing APIs so would be good to know if this really is
>>> abuse or not ;-)
>>>
>>> Cheers
>>> Jon
>>>
>>> [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
>>
>> Interesting.
>>
>> This is really something that the core DT and GPIO and IRQ maintainers
>> should weigh in on. Hence, changing them from Cc: to To: in this message
>> and/or adding them.
> 
> So I guess it is OK for a driver or DT node to use a GPIO as IRQ
> *only*, and then have the GPIO requested implicitly through the
> xlate function in the irqdomain.
> 
> It's the use cases where one and the same driver tries to use the
> same GPIO line *also* by requesting it using gpio_request()
> that madness starts. In such cases the driver deserves to have
> an error thrown back at it, as it definately knows the GPIO pin
> and can be refactored to use gpio_to_irq() to translate it into
> an IRQ rather than having it implicitly done in the xlate function.
> 
> You will just have to give the xlate function information about which
> GPIOs are used as IRQs only, and only request the GPIO on these.

That should not be necessary. The xlate is only called in the case where
the gpio controller is declared as the interrupt controller in the
device tree source ...

eth@0 {
	compatible = "ks8851";
	...
	interrupt-parent = <&gpio2>;
	interrupts = <2 8>; /* gpio line 34, low triggered */
	...
};

So in the case where you have a driver call gpio_request() and then
request_irq(), in the device tree source, the gpio controller will not
be declared as an interrupt controller. It will just list the gpios ...

sdhci@c8000200 {
	...
	cd-gpios = <&gpio 69 0>; /* gpio PI5 */
	wp-gpios = <&gpio 57 0>; /* gpio PH1 */
	...
};

> And I recently suggested a mechanism to do that, and that is
> what I called GPIO input-hogs, which means that you mark
> (e.g. from the device tree) at probe() of the gpiochip which GPIOs
> will only be used as inputs, so as to generate IRQs.
> 
> This is perfectly OS-neutral information about the how the
> hardware is set up in the system.
> 
> If you only allow these hogges inputs to be translated and
> requested in the xlate function, everything goes smooth.
> 
> It all comes back to this: keep all applicable knowledge in the
> system, so it can make proper decisions, don't try to create
> mechanisms that will half-guess things.

Agreed. However the xlate looks like a good place to request the gpio
without needing to add these "input-hogs" or any other platform code. So
if this is not considered abuse of the xlate, then it seems like an
ideal solution. I have been also implementing a generic xlate so that we
don't need to have a xlate for each platform as well.

Cheers
Jon

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Jon Hunter]

On 04/26/2013 02:31 AM, Linus Walleij wrote:
> On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas
> <martinez.javier@gmail.com> wrote:
> 
> So:
> 
>> +static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
>> +                                     struct device_node *ctrlr,
>> +                                     const u32 *intspec, unsigned int intsize,
>> +                                     irq_hw_number_t *out_hwirq,
>> +                                     unsigned int *out_type)
>> +{
>> +       int ret;
>> +       struct gpio_bank *bank = d->host_data;
>> +       int gpio = bank->chip.base + intspec[0];
>> +
>> +       if (WARN_ON(intsize < 2))
>> +               return -EINVAL;
>> +
>> +       ret = gpio_request_one(gpio, GPIOF_IN, ctrlr->full_name);
>> +       if (ret)
>> +               return ret;
> 
> So how to figure out if a device is already requesting this GPIO
> on some orthogonal axis?

I really don't think that is necessary. Hopefully, my other email [1]
elaborates on why. Let me know if this makes sense.

Cheers
Jon

[1] http://marc.info/?l=linux-arm-kernel&m=136701158117966&w=1

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Fri, Apr 26, 2013 at 11:25 PM, Jon Hunter <jon-hunter-l0cyMroinI0@public.gmane.org> wrote:
> On 04/26/2013 02:27 AM, Linus Walleij wrote:

>> You will just have to give the xlate function information about which
>> GPIOs are used as IRQs only, and only request the GPIO on these.
>
> That should not be necessary. The xlate is only called in the case where
> the gpio controller is declared as the interrupt controller in the
> device tree source ...

Sorry I don't understand this. Irqdomains are not device tree
concepts. Why would it only be called if it was declared such
or such in the device tree?

> eth@0 {
>         compatible = "ks8851";
>         ...
>         interrupt-parent = <&gpio2>;
>         interrupts = <2 8>; /* gpio line 34, low triggered */
>         ...
> };
>
> So in the case where you have a driver call gpio_request() and then
> request_irq(), in the device tree source, the gpio controller will not
> be declared as an interrupt controller. It will just list the gpios ...

And how do you get from there to the conclusion that
gpio_to_irq() will not ever be called, maybe by
some other driver than the ethernet?

> sdhci@c8000200 {
>         ...
>         cd-gpios = <&gpio 69 0>; /* gpio PI5 */
>         wp-gpios = <&gpio 57 0>; /* gpio PH1 */
>         ...
> };

Like if this driver wants to set up an IRQ?

>> It all comes back to this: keep all applicable knowledge in the
>> system, so it can make proper decisions, don't try to create
>> mechanisms that will half-guess things.
>
> Agreed. However the xlate looks like a good place to request the gpio
> without needing to add these "input-hogs" or any other platform code. So
> if this is not considered abuse of the xlate, then it seems like an
> ideal solution. I have been also implementing a generic xlate so that we
> don't need to have a xlate for each platform as well.

I'm still not following. Maybe I'm just too stupid...

How do you *avoid* requesting the GPIO in the translation
for a certain IRQ if the driver first issues gpio_request() and
then goes on to perform gpio_to_irq() itself?

Yours,
Linus Walleij

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Grant Likely]

On Fri, 26 Apr 2013 16:31:24 -0500, Jon Hunter <jon-hunter@ti.com> wrote:
> 
> On 04/26/2013 02:31 AM, Linus Walleij wrote:
> > On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas
> > <martinez.javier@gmail.com> wrote:
> > 
> > So:
> > 
> >> +static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
> >> +                                     struct device_node *ctrlr,
> >> +                                     const u32 *intspec, unsigned int intsize,
> >> +                                     irq_hw_number_t *out_hwirq,
> >> +                                     unsigned int *out_type)
> >> +{
> >> +       int ret;
> >> +       struct gpio_bank *bank = d->host_data;
> >> +       int gpio = bank->chip.base + intspec[0];
> >> +
> >> +       if (WARN_ON(intsize < 2))
> >> +               return -EINVAL;
> >> +
> >> +       ret = gpio_request_one(gpio, GPIOF_IN, ctrlr->full_name);
> >> +       if (ret)
> >> +               return ret;
> > 
> > So how to figure out if a device is already requesting this GPIO
> > on some orthogonal axis?
> 
> I really don't think that is necessary. Hopefully, my other email [1]
> elaborates on why. Let me know if this makes sense.

I would agree here. If the irq specified happens to be a GPIO; then the
onus is on the GPIO/IRQ controller driver to make sure that GPIO is
actually set up to work as an IRQ.

g.

Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Linus Walleij]

On Tue, Jun 11, 2013 at 11:25 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Fri, 26 Apr 2013 16:31:24 -0500, Jon Hunter <jon-hunter@ti.com> wrote:
>>
>> On 04/26/2013 02:31 AM, Linus Walleij wrote:
>> > On Wed, Apr 17, 2013 at 2:41 AM, Javier Martinez Canillas
>> > <martinez.javier@gmail.com> wrote:
>> >
>> > So:
>> >
>> >> +static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
>> >> +                                     struct device_node *ctrlr,
>> >> +                                     const u32 *intspec, unsigned int intsize,
>> >> +                                     irq_hw_number_t *out_hwirq,
>> >> +                                     unsigned int *out_type)
>> >> +{
>> >> +       int ret;
>> >> +       struct gpio_bank *bank = d->host_data;
>> >> +       int gpio = bank->chip.base + intspec[0];
>> >> +
>> >> +       if (WARN_ON(intsize < 2))
>> >> +               return -EINVAL;
>> >> +
>> >> +       ret = gpio_request_one(gpio, GPIOF_IN, ctrlr->full_name);
>> >> +       if (ret)
>> >> +               return ret;
>> >
>> > So how to figure out if a device is already requesting this GPIO
>> > on some orthogonal axis?
>>
>> I really don't think that is necessary. Hopefully, my other email [1]
>> elaborates on why. Let me know if this makes sense.
>
> I would agree here. If the irq specified happens to be a GPIO; then the
> onus is on the GPIO/IRQ controller driver to make sure that GPIO is
> actually set up to work as an IRQ.

Hm, re-reading this I guess the gpio_request_one() will block
others from using the pin for anything else.

Isn't that the answer to my actual question...?

It seems more like I was asking a pretty stupid question
actually.

Yours,
Linus Walleij