GPIO OMAP driver changes for v3.16

PATCH New Javier Martinez Canillas 5 participants 19 messages Apr 6, 2014 – Apr 23, 2014

Subsystems (2)

GPIO OMAP GPIO driver

lore

Share & tools

lore

Javier Martinez Canillas

author

2014-04-06 14:58 UTC #0 lore raw

cover GPIO OMAP driver changes for v3.16

Hi Linus,

Now that you have sent your changes for v3.15 to Torvalds, here are some changes for the OMAP GPIO driver targeted to v3.16. Mostly improvements so nothing here is -rc material.

The biggest change is Patch 4 that converts the driver to use the newly introduced generic irqchip helpers in the gpiolib core which allows to remove some driver specific logic that should really be generic.

Aaro and Paul,

I've just build tested using omap1_defconfig since I don't have any OMAP1 board to test. It would be nice if you could test using any of your boards to see if this patch-set does not introduce a regression. Specially on those using MPUIO interrupt generation since I'm not familiar with it and I could get something wrong while studying the code.

I prefer this set to not be merged until we have some testing on OMAP1 SoCs.

Santosh and Kevin,

If you agree I would like to be added to the list of maintainers for this driver since that is what I've been doing lately anyways and that way tools like get_maintainer.pl will hint people posting patches that I'm interested in being cc'ed for this driver.

The patch-set is composed of the following patches:

Javier Martinez Canillas (5):
gpio: omap: convert to use irq_domain_add_simple()
gpio: omap: check gpiochip_add() return value
gpio: omap: add a GPIO_OMAP option instead of using ARCH_OMAP
gpio: omap: convert driver to use gpiolib irqchip
MAINTAINERS: update GPIO OMAP driver entry

 MAINTAINERS              |   1 +
 drivers/gpio/Kconfig     |   8 +++
 drivers/gpio/Makefile    |   2 +-
 drivers/gpio/gpio-omap.c | 128 +++++++++++++++++++++++------------------------
 4 files changed, 73 insertions(+), 66 deletions(-)

Thanks a lot and best regards,
Javier

Signature · 1 line

1.9.0

Javier Martinez Canillas

author

2014-04-06 14:58 UTC #1 lore raw

PATCH 1/5 gpio: omap: convert to use irq_domain_add_simple()

The GPIO OMAP driver supports different OMAP SoC families and not all of them have the needed support to use the linear IRQ domain mapping like OMAP1 that use the legacy domain mapping.

But this special check is not necessary since the simple IRQ domain mapping is able to handle both cases. Having a zero IRQ offset will be interpreted as a linear domain case while a non-zero value will be interpreted as a legacy domain case.

Signed-off-by Javier Martinez Canillas <[email protected]>

---
 drivers/gpio/gpio-omap.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

drivers/gpio/gpio-omap.c +4 ~~-11~~

index 19b886c..3ee9b8d 100644
@@ -1138,9 +1138,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	const struct omap_gpio_platform_data *pdata;
 	struct resource *res;
 	struct gpio_bank *bank;
-#ifdef CONFIG_ARCH_OMAP1
-	int irq_base;
-#endif
+	int irq_base = 0;
 
 	match = of_match_device(of_match_ptr(omap_gpio_match), dev);

@@ -1185,21 +1183,16 @@ static int omap_gpio_probe(struct platform_device *pdev)
 #ifdef CONFIG_ARCH_OMAP1
 	/*
 	 * REVISIT: Once we have OMAP1 supporting SPARSE_IRQ, we can drop
-	 * irq_alloc_descs() and irq_domain_add_legacy() and just use a
-	 * linear IRQ domain mapping for all OMAP platforms.
+	 * irq_alloc_descs() since a base IRQ offset will no longer be needed.
 	 */
 	irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
 	if (irq_base < 0) {
 		dev_err(dev, "Couldn't allocate IRQ numbers\n");
 		return -ENODEV;
 	}
-
-	bank->domain = irq_domain_add_legacy(node, bank->width, irq_base,
-					     0, &irq_domain_simple_ops, NULL);
-#else
-	bank->domain = irq_domain_add_linear(node, bank->width,
-					     &irq_domain_simple_ops, NULL);
 #endif
+	bank->domain = irq_domain_add_simple(node, bank->width, irq_base,
+					     &irq_domain_simple_ops, NULL);
 	if (!bank->domain) {
 		dev_err(dev, "Couldn't register an IRQ domain\n");
 		return -ENODEV;

Signature · 1 line

1.9.0

Santosh Shilimkar

2014-04-10 17:35 UTC #2 lore raw

PATCH 1/5 gpio: omap: convert to use irq_domain_add_simple()

Quoted 45 lines · Javier Martinez Canillas

The GPIO OMAP driver supports different OMAP SoC families and not all of them have the needed support to use the linear IRQ domain mapping like OMAP1 that use the legacy domain mapping.

Signed-off-by Javier Martinez Canillas <[email protected]>

---
 drivers/gpio/gpio-omap.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

drivers/gpio/gpio-omap.c +2 -7

index 19b886c..3ee9b8d 100644
@@ -1138,9 +1138,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	const struct omap_gpio_platform_data *pdata;
 	struct resource *res;
 	struct gpio_bank *bank;
-#ifdef CONFIG_ARCH_OMAP1
-	int irq_base;
-#endif
+	int irq_base = 0;
 
 	match = of_match_device(of_match_ptr(omap_gpio_match), dev);

@@ -1185,21 +1183,16 @@ static int omap_gpio_probe(struct platform_device *pdev)
 #ifdef CONFIG_ARCH_OMAP1
 	/*
 	 * REVISIT: Once we have OMAP1 supporting SPARSE_IRQ, we can drop
-	 * irq_alloc_descs() and irq_domain_add_legacy() and just use a
-	 * linear IRQ domain mapping for all OMAP platforms.
+	 * irq_alloc_descs() since a base IRQ offset will no longer be needed.
 	 */
 	irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
 	if (irq_base < 0) {
 		dev_err(dev, "Couldn't allocate IRQ numbers\n");
 		return -ENODEV;
 	}
-
-	bank->domain = irq_domain_add_legacy(node, bank->width, irq_base,

drivers/gpio/gpio-omap.c static int omap_gpio_probe(struct platform_device *pdev) · line 1198

Quoted 10 lines

drivers/gpio/gpio-omap.c +2 -4

-					     0, &irq_domain_simple_ops, NULL);
-#else
-	bank->domain = irq_domain_add_linear(node, bank->width,
-					     &irq_domain_simple_ops, NULL);
 #endif
+	bank->domain = irq_domain_add_simple(node, bank->width, irq_base,
+					     &irq_domain_simple_ops, NULL);
 	if (!bank->domain) {
 		dev_err(dev, "Couldn't register an IRQ domain\n");
 		return -ENODEV;

Looks good.

Acked-by Santosh Shilimkar <[email protected]>

Javier Martinez Canillas

author

2014-04-06 14:58 UTC #3 lore raw

PATCH 2/5 gpio: omap: check gpiochip_add() return value

In reply to Javier Martinez Canillas

The gpiochip_add() function can fail if the chip cannot be registered so the return value has to be checked and the error propagated in case it happens.

Signed-off-by Javier Martinez Canillas <[email protected]>

---
 drivers/gpio/gpio-omap.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

drivers/gpio/gpio-omap.c +13 -3

index 3ee9b8d..e717888 100644
@@ -1081,10 +1081,11 @@ omap_mpuio_alloc_gc(struct gpio_bank *bank, unsigned int irq_start,
 			       IRQ_NOREQUEST | IRQ_NOPROBE, 0);
 }
 
-static void omap_gpio_chip_init(struct gpio_bank *bank)
+static int omap_gpio_chip_init(struct gpio_bank *bank)
 {
 	int j;
 	static int gpio;
+	int ret;
 
 	/*
 	 * REVISIT eventually switch from OMAP-specific gpio structs

@@ -1110,7 +1111,11 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
 	}
 	bank->chip.ngpio = bank->width;
 
-	gpiochip_add(&bank->chip);
+	ret = gpiochip_add(&bank->chip);
+	if (ret) {
+		dev_err(bank->dev, "Could not register gpio chip\n", ret);
+		return ret;
+	}
 
 	for (j = 0; j < bank->width; j++) {
 		int irq = irq_create_mapping(bank->domain, j);

@@ -1139,6 +1144,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct gpio_bank *bank;
 	int irq_base = 0;
+	int ret;
 
 	match = of_match_device(of_match_ptr(omap_gpio_match), dev);

@@ -1223,7 +1229,11 @@ static int omap_gpio_probe(struct platform_device *pdev)
 		mpuio_init(bank);
 
 	omap_gpio_mod_init(bank);
-	omap_gpio_chip_init(bank);
+
+	ret = omap_gpio_chip_init(bank);
+	if (ret)
+		return ret;
+
 	omap_gpio_show_rev(bank);
 
 	pm_runtime_put(bank->dev);

Signature · 1 line

1.9.0

Santosh Shilimkar

2014-04-10 17:36 UTC #4 lore raw

PATCH 2/5 gpio: omap: check gpiochip_add() return value

Quoted 6 lines · Javier Martinez Canillas

The gpiochip_add() function can fail if the chip cannot be registered so the return value has to be checked and the error propagated in case it happens.
Signed-off-by Javier Martinez Canillas <[email protected]>
---

Acked-by Santosh Shilimkar <[email protected]>

Javier Martinez Canillas

author

2014-04-06 14:58 UTC #5 lore raw

PATCH 3/5 gpio: omap: add a GPIO_OMAP option instead of using ARCH_OMAP

In reply to Javier Martinez Canillas

The ARCH_OMAP config option was used to built the GPIO OMAP driver but this is not consistent with the rest of the GPIO drivers that have their own Kconfig option.

Also, this make it harder to add dependencies or reverse dependencies (i.e: select) since that would mean touching the sub-arch config option.

So is better to add a boolean Kconfig option for this driver that defaults to true if ARCH_OMAP is enabled.

Signed-off-by Javier Martinez Canillas <[email protected]>

---
 drivers/gpio/Kconfig  | 7 +++++++
 drivers/gpio/Makefile | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

drivers/gpio/Kconfig +7 -0

index 92d8e9a..bfe5cf0 100644
@@ -243,6 +243,13 @@ config GPIO_OCTEON
 	  Say yes here to support the on-chip GPIO lines on the OCTEON
 	  family of SOCs.
 
+config GPIO_OMAP
+	bool "TI OMAP GPIO support"
+	default y if ARCH_OMAP
+	depends on ARM && ARCH_OMAP
+	help
+	  Say yes here to enable GPIO support for TI OMAP SoCs.
+
 config GPIO_PL061
 	bool "PrimeCell PL061 GPIO support"
 	depends on ARM_AMBA

drivers/gpio/Makefile +1 -1

index 6309aff..d10f6a9 100644
@@ -58,7 +58,7 @@ obj-$(CONFIG_GPIO_MVEBU)        += gpio-mvebu.o
 obj-$(CONFIG_GPIO_MXC)		+= gpio-mxc.o
 obj-$(CONFIG_GPIO_MXS)		+= gpio-mxs.o
 obj-$(CONFIG_GPIO_OCTEON)	+= gpio-octeon.o
-obj-$(CONFIG_ARCH_OMAP)		+= gpio-omap.o
+obj-$(CONFIG_GPIO_OMAP)		+= gpio-omap.o
 obj-$(CONFIG_GPIO_PCA953X)	+= gpio-pca953x.o
 obj-$(CONFIG_GPIO_PCF857X)	+= gpio-pcf857x.o
 obj-$(CONFIG_GPIO_PCH)		+= gpio-pch.o

Signature · 1 line

1.9.0

Santosh Shilimkar

2014-04-10 17:37 UTC #6 lore raw

PATCH 3/5 gpio: omap: add a GPIO_OMAP option instead of using ARCH_OMAP

Quoted 8 lines · Javier Martinez Canillas

The ARCH_OMAP config option was used to built the GPIO OMAP driver but this is not consistent with the rest of the GPIO drivers that have their own Kconfig option.
Also, this make it harder to add dependencies or reverse dependencies (i.e: select) since that would mean touching the sub-arch config option.

Quoted 5 lines

So is better to add a boolean Kconfig option for this driver that defaults to true if ARCH_OMAP is enabled.
Signed-off-by Javier Martinez Canillas <[email protected]>
---

Acked-by Santosh Shilimkar <[email protected]>

Javier Martinez Canillas

author

2014-04-06 14:58 UTC #7 lore raw

PATCH 4/5 gpio: omap: convert driver to use gpiolib irqchip

In reply to Javier Martinez Canillas

Converts the GPIO OMAP driver to register its chained irq handler and irqchip using the helpers in the gpiolib core.

Signed-off-by Javier Martinez Canillas <[email protected]>

---
 drivers/gpio/Kconfig     |   1 +
 drivers/gpio/gpio-omap.c | 107 ++++++++++++++++++++++-------------------------
 2 files changed, 52 insertions(+), 56 deletions(-)

drivers/gpio/Kconfig +1 -0

index bfe5cf0..2092b1d 100644
@@ -247,6 +247,7 @@ config GPIO_OMAP
 	bool "TI OMAP GPIO support"
 	default y if ARCH_OMAP
 	depends on ARM && ARCH_OMAP
+	select GPIOLIB_IRQCHIP
 	help
 	  Say yes here to enable GPIO support for TI OMAP SoCs.

drivers/gpio/gpio-omap.c +51 ~~-56~~

244 lines not on this page · raw message

Signature · 1 line

1.9.0

Santosh Shilimkar

2014-04-10 17:39 UTC #8 lore raw

PATCH 4/5 gpio: omap: convert driver to use gpiolib irqchip

Quoted 194 lines · Javier Martinez Canillas

Converts the GPIO OMAP driver to register its chained irq handler and irqchip using the helpers in the gpiolib core.

Signed-off-by Javier Martinez Canillas <[email protected]>

---
 drivers/gpio/Kconfig     |   1 +
 drivers/gpio/gpio-omap.c | 107 ++++++++++++++++++++++-------------------------
 2 files changed, 52 insertions(+), 56 deletions(-)

drivers/gpio/Kconfig +1 -0

index bfe5cf0..2092b1d 100644
@@ -247,6 +247,7 @@ config GPIO_OMAP
 	bool "TI OMAP GPIO support"
 	default y if ARCH_OMAP
 	depends on ARM && ARCH_OMAP
+	select GPIOLIB_IRQCHIP
 	help
 	  Say yes here to enable GPIO support for TI OMAP SoCs.

drivers/gpio/gpio-omap.c +21 ~~-28~~

index e717888..8cc9e91 100644
@@ -24,7 +24,6 @@
 #include <linux/pm.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/gpio.h>
 #include <linux/platform_data/gpio-omap.h>

@@ -52,7 +51,6 @@ struct gpio_bank {
 	struct list_head node;
 	void __iomem *base;
 	u16 irq;
-	struct irq_domain *domain;
 	u32 non_wakeup_gpios;
 	u32 enabled_non_wakeup_gpios;
 	struct gpio_regs context;

@@ -95,11 +93,10 @@ static int irq_to_gpio(struct gpio_bank *bank, unsigned int gpio_irq)
 	return bank->chip.base + gpio_irq;
 }
 
-static int omap_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+static inline struct gpio_bank *_irq_data_get_bank(struct irq_data *d)
 {
-	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
-
-	return irq_find_mapping(bank->domain, offset);
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	return container_of(chip, struct gpio_bank, chip);
 }
 
 static void _set_gpio_direction(struct gpio_bank *bank, int gpio, int is_input)

@@ -479,7 +476,7 @@ static int gpio_is_input(struct gpio_bank *bank, int mask)
 
 static int gpio_irq_type(struct irq_data *d, unsigned type)
 {
-	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	struct gpio_bank *bank = _irq_data_get_bank(d);
 	unsigned gpio = 0;
 	int retval;
 	unsigned long flags;

@@ -514,14 +511,6 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 		return -EINVAL;
 	}
 
-	retval = gpio_lock_as_irq(&bank->chip, offset);
-	if (retval) {
-		dev_err(bank->dev, "unable to lock offset %d for IRQ\n",
-			offset);
-		spin_unlock_irqrestore(&bank->lock, flags);
-		return retval;
-	}
-
 	bank->irq_usage |= 1 << GPIO_INDEX(bank, gpio);
 	spin_unlock_irqrestore(&bank->lock, flags);

@@ -664,7 +653,7 @@ static void _reset_gpio(struct gpio_bank *bank, int gpio)
 /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */
 static int gpio_wake_enable(struct irq_data *d, unsigned int enable)
 {
-	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	struct gpio_bank *bank = _irq_data_get_bank(d);
 	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 
 	return _set_gpio_wakeup(bank, gpio, enable);

@@ -732,11 +721,12 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	unsigned int bit;
 	struct gpio_bank *bank;
 	int unmasked = 0;
-	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+	struct gpio_chip *chip = irq_get_handler_data(irq);
 
-	chained_irq_enter(chip, desc);
+	chained_irq_enter(irqchip, desc);
 
-	bank = irq_get_handler_data(irq);
+	bank = container_of(chip, struct gpio_bank, chip);
 	isr_reg = bank->base + bank->regs->irqstatus;
 	pm_runtime_get_sync(bank->dev);

@@ -764,7 +754,7 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 		configured, we could unmask GPIO bank interrupt immediately */
 		if (!level_mask && !unmasked) {
 			unmasked = 1;
-			chained_irq_exit(chip, desc);
+			chained_irq_exit(irqchip, desc);
 		}
 
 		if (!isr)

@@ -784,7 +774,8 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 			if (bank->toggle_mask & (1 << bit))
 				_toggle_gpio_edge_triggering(bank, bit);
 
-			generic_handle_irq(irq_find_mapping(bank->domain, bit));
+			generic_handle_irq(irq_find_mapping(bank->chip.irqdomain,
+							    bit));
 		}
 	}
 	/* if bank has any level sensitive GPIO pin interrupt

@@ -793,13 +784,13 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	interrupt */
 exit:
 	if (!unmasked)
-		chained_irq_exit(chip, desc);
+		chained_irq_exit(irqchip, desc);
 	pm_runtime_put(bank->dev);
 }
 
 static void gpio_irq_shutdown(struct irq_data *d)
 {
-	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	struct gpio_bank *bank = _irq_data_get_bank(d);
 	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned long flags;
 	unsigned offset = GPIO_INDEX(bank, gpio);

@@ -821,7 +812,7 @@ static void gpio_irq_shutdown(struct irq_data *d)
 
 static void gpio_ack_irq(struct irq_data *d)
 {
-	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	struct gpio_bank *bank = _irq_data_get_bank(d);
 	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 
 	_clear_gpio_irqstatus(bank, gpio);

@@ -829,7 +820,7 @@ static void gpio_ack_irq(struct irq_data *d)
 
 static void gpio_mask_irq(struct irq_data *d)
 {
-	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	struct gpio_bank *bank = _irq_data_get_bank(d);
 	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned long flags;

@@ -841,7 +832,7 @@ static void gpio_mask_irq(struct irq_data *d)
 
 static void gpio_unmask_irq(struct irq_data *d)
 {
-	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	struct gpio_bank *bank = _irq_data_get_bank(d);
 	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned int irq_mask = GPIO_BIT(bank, gpio);
 	u32 trigger = irqd_get_trigger_type(d);

@@ -1085,6 +1076,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank)
 {
 	int j;
 	static int gpio;
+	int irq_base = 0;
 	int ret;
 
 	/*

@@ -1098,7 +1090,6 @@ static int omap_gpio_chip_init(struct gpio_bank *bank)
 	bank->chip.direction_output = gpio_output;
 	bank->chip.set_debounce = gpio_debounce;
 	bank->chip.set = gpio_set;
-	bank->chip.to_irq = omap_gpio_to_irq;
 	if (bank->is_mpuio) {
 		bank->chip.label = "mpuio";
 		if (bank->regs->wkup_en)

@@ -1113,24 +1104,46 @@ static int omap_gpio_chip_init(struct gpio_bank *bank)
 
 	ret = gpiochip_add(&bank->chip);
 	if (ret) {
-		dev_err(bank->dev, "Could not register gpio chip\n", ret);
+		dev_err(bank->dev, "Could not register gpio chip %d\n", ret);
 		return ret;
 	}
 
+#ifdef CONFIG_ARCH_OMAP1
+	/*

drivers/gpio/gpio-omap.c static int omap_gpio_chip_init(struct gpio_bank *bank) · line 1122

Quoted 10 lines

drivers/gpio/gpio-omap.c +10 -0

+	 * REVISIT: Once we have OMAP1 supporting SPARSE_IRQ, we can drop
+	 * irq_alloc_descs() since a base IRQ offset will no longer be needed.
+	 */
+	irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
+	if (irq_base < 0) {
+		dev_err(bank->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+#endif
+

Do we still need above after first patch ? Would be good to get rid of above ugly #ifdef on the middle of the code.

Linus Walleij

2014-04-10 17:45 UTC #9 lore raw

PATCH 4/5 gpio: omap: convert driver to use gpiolib irqchip

Quoted 16 lines · Santosh Shilimkar

drivers/gpio/gpio-omap.c static int omap_gpio_chip_init(struct gpio_bank *bank) · line 1122
Quoted 12 lines · Javier Martinez Canillas
drivers/gpio/gpio-omap.c +12 -0
+#ifdef CONFIG_ARCH_OMAP1
+     /*
+      * REVISIT: Once we have OMAP1 supporting SPARSE_IRQ, we can drop
+      * irq_alloc_descs() since a base IRQ offset will no longer be needed.
+      */
+     irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
+     if (irq_base < 0) {
+             dev_err(bank->dev, "Couldn't allocate IRQ numbers\n");
+             return -ENODEV;
+     }
+#endif
+
Do we still need above after first patch ? Would be good to get rid of above ugly #ifdef on the middle of the code.

I don't think so actually, simple irqdomain adds descriptors for irqbase != 0, and the gpiochip irqchip helpers call irq_create_mapping() on all offsets.

Preferably a separate patch on top of this removing that code though so it's bisectable.

Yours,
Linus Walleij

Javier Martinez Canillas

2014-04-10 18:58 UTC #10 lore raw

PATCH 4/5 gpio: omap: convert driver to use gpiolib irqchip

Hello Linus and Santosh,

Quoted 4 lines · Linus Walleij

Quoted 1 line · Santosh Shilimkar
On Sunday 06 April 2014 10:58 AM, Javier Martinez Canillas wrote:

Quoted 14 lines

Quoted 6 lines
Quoted 6 lines
drivers/gpio/gpio-omap.c +6 -0
+#ifdef CONFIG_ARCH_OMAP1
+     /*
+      * REVISIT: Once we have OMAP1 supporting SPARSE_IRQ, we can drop
+      * irq_alloc_descs() since a base IRQ offset will no longer be needed.
+      */
+     irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
Quoted 8 lines
drivers/gpio/gpio-omap.c static int omap_gpio_chip_init(struct gpio_bank *bank) · line 1122
Quoted 6 lines
drivers/gpio/gpio-omap.c +6 -0
+     if (irq_base < 0) {
+             dev_err(bank->dev, "Couldn't allocate IRQ numbers\n");
+             return -ENODEV;
+     }
+#endif
+
Do we still need above after first patch ? Would be good to get rid of above ugly #ifdef on the middle of the code.

When working on this series I tried to remove the #ifdef but as far as I understood is not currently possible until OMAP1 supports sparse irq numbering.

Not so long ago the GPIO OMAP driver used the legacy domain mapping for all OMAP SoCs and Jon Hunter converted to use linear domain mapping on commit ede4d7a ("gpio/omap: convert gpio irq domain to linear mapping").

Unfortunately that change caused a regression on OMAP1 platforms as reported by Aaro [0] so I had to partially revert the linear domain mapping for OMAP1 platforms on commit 397ead ("gpio/omap: don't use linear domain mapping for OMAP1") introducing this ugly ifdefery in the middle of the code.

Quoted 5 lines

I don't think so actually, simple irqdomain adds descriptors for irqbase != 0, and the gpiochip irqchip helpers call irq_create_mapping() on all offsets.

Looking at irq_domain_add_simple() [1] I see that it only calls irq_alloc_descs() and irq_domain_associate_many() if first_irq > 0 and CONFIG_SPARSE_IRQ is enabled (which is not the case for omap1_defconfig).

So, if I got this correctly removing the #ifdef for OMAP1 and calling irq_domain_add_simple() is functionally equivalent to what Jon's patch did that broke omap1. That is would have the same effect that just calling irq_domain_add_linear() [2] for OMAP1.

Quoted 3 lines

Preferably a separate patch on top of this removing that code though so it's bisectable.

If I remember correctly we did that partial revert because we were in a -rc cycle and I didn't have time to investigate why irq_domain_add_linear() does not work on omap1. I could try to do this as a follow up patch but unfortunately I don't have access to any omap1 platform to do further debug.

Please let me know if I got something wrong while looking at the code.

Quoted 3 lines

Yours,
Linus Walleij
--

Thanks a lot and best regards,
Javier

[0]: http://www.mail-archive.com/linux-omap at vger.kernel.org/msg89005.html [1]: http://lxr.free-electrons.com/source/kernel/irq/irqdomain.c#L123
[2]: http://lxr.free-electrons.com/source/include/linux/irqdomain.h#L139

Javier Martinez Canillas

author

2014-04-06 14:58 UTC #11 lore raw

PATCH 5/5 MAINTAINERS: update GPIO OMAP driver entry

In reply to Javier Martinez Canillas

I've been maintaining this driver by fixing all issues found while migrating OMAP2+ towards Device Tree based booting and keeping it up-to-date by using the latest infraestructure that is provided by the GPIO subsystem.

It would be nice if people know that I care about this driver and put me in copy when sending patches for it.

Signed-off-by Javier Martinez Canillas <[email protected]>

---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

MAINTAINERS +1 -0

index f728ac2..fc0972e 100644
@@ -6374,6 +6374,7 @@ F:	drivers/usb/*/*omap*
 F:	arch/arm/*omap*/usb*
 
 OMAP GPIO DRIVER
+M:	Javier Martinez Canillas <[email protected]>
 M:	Santosh Shilimkar <[email protected]>
 M:	Kevin Hilman <[email protected]>
 L:	linux-omap at vger.kernel.org

Signature · 1 line

1.9.0

Linus Walleij

2014-04-10 17:29 UTC #12 lore raw

cover GPIO OMAP driver changes for v3.16

In reply to Javier Martinez Canillas

Quoted 3 lines · Javier Martinez Canillas

Now that you have sent your changes for v3.15 to Torvalds, here are some changes for the OMAP GPIO driver targeted to v3.16. Mostly improvements so nothing here is -rc material.

I like this series so I have applied them for v3.16, pending some ACK from Kevin &| Santosh.

Quoted 3 lines

The biggest change is Patch 4 that converts the driver to use the newly introduced generic irqchip helpers in the gpiolib core which allows to remove some driver specific logic that should really be generic.

Excellent, I will take a closer look at this, it seems there is some cruft still in but let me look.

Yours,
Linus Walleij

Aaro Koskinen

2014-04-10 19:30 UTC #13 lore raw

cover GPIO OMAP driver changes for v3.16

Hi,

Quoted 9 lines · Linus Walleij

Quoted 3 lines · Javier Martinez Canillas
Now that you have sent your changes for v3.15 to Torvalds, here are some changes for the OMAP GPIO driver targeted to v3.16. Mostly improvements so nothing here is -rc material.
I like this series so I have applied them for v3.16, pending some ACK from Kevin &| Santosh.

I tried these patches on OMAP1 on top of today's Torvalds git (4ba85265790ba3681deeaf73f018c0eb829a7341).

On Amstrad E3 I'm getting the following logs:

[    0.156491] omap_gpio omap_gpio.0: Runtime PM disabled, clock forced on.
[    0.164604] genirq: Setting trigger mode 0 for irq 64 failed (gpio_irq_type+0x0/0x1f0)
[    0.165418] genirq: Setting trigger mode 0 for irq 65 failed (gpio_irq_type+0x0/0x1f0)
[    0.166133] genirq: Setting trigger mode 0 for irq 66 failed (gpio_irq_type+0x0/0x1f0)
[    0.166838] genirq: Setting trigger mode 0 for irq 67 failed (gpio_irq_type+0x0/0x1f0)
[...]
[    0.182856] genirq: Setting trigger mode 0 for irq 79 failed (gpio_irq_type+0x0/0x1f0)
[    0.186887] omap_gpio omap_gpio.1: Could not get gpio dbck
[    0.189308] genirq: Setting trigger mode 0 for irq 95 failed (gpio_irq_type+0x0/0x1f0)
[...]
[    0.203121] genirq: Setting trigger mode 0 for irq 110 failed (gpio_irq_type+0x0/0x1f0)

However it still seems to work. The serio is only GPIO IRQ and it triggers when I press the external keyboard.

The same happens also on Nokia 770:

[    0.118896] genirq: Setting trigger mode 0 for irq 128 failed (gpio_irq_type+0x0/0x220)
[    0.119201] genirq: Setting trigger mode 0 for irq 129 failed (gpio_irq_type+0x0/0x220)
[...]
[    0.124999] genirq: Setting trigger mode 0 for irq 143 failed (gpio_irq_type
+0x0/0x220)
[    0.126831] omap_gpio omap_gpio.1: Could not get gpio dbck
[    0.127258] OMAP GPIO hardware version 1.1
[    0.127624] omap_gpio omap_gpio.2: Could not get gpio dbck
[    0.128204] omap_gpio omap_gpio.3: Could not get gpio dbck
[    0.128753] omap_gpio omap_gpio.4: Could not get gpio dbck

Here also GPIO IRQs (touchscreen, Retu) still work.

Javier Martinez Canillas

2014-04-10 20:17 UTC #14 lore raw

cover GPIO OMAP driver changes for v3.16

Hello Aaro,

Thanks a lot for testing the series!

Quoted 46 lines · Aaro Koskinen

Hi,
Quoted 9 lines · Linus Walleij
Quoted 3 lines · Javier Martinez Canillas
Now that you have sent your changes for v3.15 to Torvalds, here are some changes for the OMAP GPIO driver targeted to v3.16. Mostly improvements so nothing here is -rc material.
I like this series so I have applied them for v3.16, pending some ACK from Kevin &| Santosh.
I tried these patches on OMAP1 on top of today's Torvalds git (4ba85265790ba3681deeaf73f018c0eb829a7341).
On Amstrad E3 I'm getting the following logs:
[    0.156491] omap_gpio omap_gpio.0: Runtime PM disabled, clock forced on.
[    0.164604] genirq: Setting trigger mode 0 for irq 64 failed (gpio_irq_type+0x0/0x1f0)
[    0.165418] genirq: Setting trigger mode 0 for irq 65 failed (gpio_irq_type+0x0/0x1f0)
[    0.166133] genirq: Setting trigger mode 0 for irq 66 failed (gpio_irq_type+0x0/0x1f0)
[    0.166838] genirq: Setting trigger mode 0 for irq 67 failed (gpio_irq_type+0x0/0x1f0)
[...]
[    0.182856] genirq: Setting trigger mode 0 for irq 79 failed (gpio_irq_type+0x0/0x1f0)
[    0.186887] omap_gpio omap_gpio.1: Could not get gpio dbck
[    0.189308] genirq: Setting trigger mode 0 for irq 95 failed (gpio_irq_type+0x0/0x1f0)
[...]
[    0.203121] genirq: Setting trigger mode 0 for irq 110 failed (gpio_irq_type+0x0/0x1f0)
However it still seems to work. The serio is only GPIO IRQ and it triggers when I press the external keyboard.
The same happens also on Nokia 770:
[    0.118896] genirq: Setting trigger mode 0 for irq 128 failed (gpio_irq_type+0x0/0x220)
[    0.119201] genirq: Setting trigger mode 0 for irq 129 failed (gpio_irq_type+0x0/0x220)
[...]
[    0.124999] genirq: Setting trigger mode 0 for irq 143 failed (gpio_irq_type
+0x0/0x220)
[    0.126831] omap_gpio omap_gpio.1: Could not get gpio dbck
[    0.127258] OMAP GPIO hardware version 1.1
[    0.127624] omap_gpio omap_gpio.2: Could not get gpio dbck
[    0.128204] omap_gpio omap_gpio.3: Could not get gpio dbck
[    0.128753] omap_gpio omap_gpio.4: Could not get gpio dbck

Quoted 3 lines

Here also GPIO IRQs (touchscreen, Retu) still work.
A.

I don't have those errors when booting on my DM3730 IGEPv2 board but it seems that for some reason on omap1 __irq_set_trigger() complains when IRQ_TYPE_NONE is used as a default flag when calling gpiochip_irqchip_add()

Could you please test the following patch and tell me if your board still works and makes the errors go away?

drivers/gpio/gpio-omap.c +1 -1

index 8cc9e91..5bc8aec 100644
@@ -1122,7 +1122,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank)
 
        ret = gpiochip_irqchip_add(&bank->chip, &gpio_irq_chip,
                                   irq_base, gpio_irq_handler,
-                                  IRQ_TYPE_NONE);
+                                  IRQ_TYPE_LEVEL_LOW);
 
        if (ret) {
                dev_err(bank->dev, "Couldn't add irqchip to gpiochip

%d\n", ret);

Best regards,
Javier

Aaro Koskinen

2014-04-10 21:22 UTC #15 lore raw

cover GPIO OMAP driver changes for v3.16

Hi,

Quoted 11 lines · Javier Martinez Canillas

Quoted 3 lines
The same happens also on Nokia 770:
[    0.118896] genirq: Setting trigger mode 0 for irq 128 failed (gpio_irq_type+0x0/0x220)
I don't have those errors when booting on my DM3730 IGEPv2 board but it seems that for some reason on omap1 __irq_set_trigger() complains when IRQ_TYPE_NONE is used as a default flag when calling gpiochip_irqchip_add()
Could you please test the following patch and tell me if your board still works and makes the errors go away?

Now it complains about mode 8...

[    0.118835] genirq: Setting trigger mode 8 for irq 128 failed (gpio_irq_type

+0x0/0x220)

Quoted 17 lines

drivers/gpio/gpio-omap.c +1 -1

index 8cc9e91..5bc8aec 100644
@@ -1122,7 +1122,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank)
 
        ret = gpiochip_irqchip_add(&bank->chip, &gpio_irq_chip,
                                   irq_base, gpio_irq_handler,
-                                  IRQ_TYPE_NONE);
+                                  IRQ_TYPE_LEVEL_LOW);
 
        if (ret) {
                dev_err(bank->dev, "Couldn't add irqchip to gpiochip

%d\n", ret);

Best regards,
Javier

Javier Martinez Canillas

2014-04-11 15:03 UTC #16 lore raw

cover GPIO OMAP driver changes for v3.16

Hello Aaro,

Quoted 33 lines · Aaro Koskinen

Hi,
Quoted 11 lines · Javier Martinez Canillas
Quoted 3 lines
The same happens also on Nokia 770:
[    0.118896] genirq: Setting trigger mode 0 for irq 128 failed (gpio_irq_type+0x0/0x220)
I don't have those errors when booting on my DM3730 IGEPv2 board but it seems that for some reason on omap1 __irq_set_trigger() complains when IRQ_TYPE_NONE is used as a default flag when calling gpiochip_irqchip_add()
Could you please test the following patch and tell me if your board still works and makes the errors go away?
Now it complains about mode 8...
[    0.118835] genirq: Setting trigger mode 8 for irq 128 failed (gpio_irq_type
+0x0/0x220)
A.
Quoted 11 lines
drivers/gpio/gpio-omap.c +1 -1
index 8cc9e91..5bc8aec 100644
@@ -1122,7 +1122,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank)
 
        ret = gpiochip_irqchip_add(&bank->chip, &gpio_irq_chip,
                                   irq_base, gpio_irq_handler,
-                                  IRQ_TYPE_NONE);
+                                  IRQ_TYPE_LEVEL_LOW);
 

Quoted 6 lines

Quoted 6 lines
drivers/gpio/gpio-omap.c +0 -0
        if (ret) {
                dev_err(bank->dev, "Couldn't add irqchip to gpiochip
%d\n", ret);
Best regards,
Javier

Thanks for testing. Unfortunately I'm out of ideas on why that error could be shown and I don't have a way to further debug it without an omap1 board. I wonder why that pr_err() message is shown or why it is still working when an error happens.

Maybe Linus or Santosh could give us a hint on what is happening here.

Best regards,
Javier

Linus Walleij

2014-04-22 13:00 UTC #17 lore raw

cover GPIO OMAP driver changes for v3.16

Quoted 48 lines · Javier Martinez Canillas

Hello Aaro,
Quoted 33 lines · Aaro Koskinen
Hi,
Quoted 11 lines · Javier Martinez Canillas
Quoted 3 lines
The same happens also on Nokia 770:
[    0.118896] genirq: Setting trigger mode 0 for irq 128 failed (gpio_irq_type+0x0/0x220)
I don't have those errors when booting on my DM3730 IGEPv2 board but it seems that for some reason on omap1 __irq_set_trigger() complains when IRQ_TYPE_NONE is used as a default flag when calling gpiochip_irqchip_add()
Could you please test the following patch and tell me if your board still works and makes the errors go away?
Now it complains about mode 8...
[    0.118835] genirq: Setting trigger mode 8 for irq 128 failed (gpio_irq_type
+0x0/0x220)
A.
Quoted 11 lines
drivers/gpio/gpio-omap.c +1 -1
index 8cc9e91..5bc8aec 100644
@@ -1122,7 +1122,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank)
 
        ret = gpiochip_irqchip_add(&bank->chip, &gpio_irq_chip,
                                   irq_base, gpio_irq_handler,
-                                  IRQ_TYPE_NONE);
+                                  IRQ_TYPE_LEVEL_LOW);
 
Quoted 6 lines
Quoted 6 lines
drivers/gpio/gpio-omap.c +0 -0
        if (ret) {
                dev_err(bank->dev, "Couldn't add irqchip to gpiochip
%d\n", ret);
Best regards,
Javier
Thanks for testing. Unfortunately I'm out of ideas on why that error could be shown and I don't have a way to further debug it without an omap1 board. I wonder why that pr_err() message is shown or why it is still working when an error happens.

Quoted 1 line

Maybe Linus or Santosh could give us a hint on what is happening here.

Isn't an edge IRQ more apropriate as default then?

The code contains this:

    if (!bank->regs->leveldetect0 &&
        (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
        return -EINVAL;

Meaning sometimes the banks don't support level IRQs.

Yours,
Linus Walleij

Javier Martinez Canillas

2014-04-23 21:48 UTC #18 lore raw

cover GPIO OMAP driver changes for v3.16

In reply to Aaro Koskinen

Hello Aaro,

Quoted 15 lines · Aaro Koskinen

Hi,
Quoted 11 lines · Javier Martinez Canillas
Quoted 3 lines
The same happens also on Nokia 770:
[    0.118896] genirq: Setting trigger mode 0 for irq 128 failed (gpio_irq_type+0x0/0x220)
I don't have those errors when booting on my DM3730 IGEPv2 board but it seems that for some reason on omap1 __irq_set_trigger() complains when IRQ_TYPE_NONE is used as a default flag when calling gpiochip_irqchip_add()
Could you please test the following patch and tell me if your board still works and makes the errors go away?

Quoted 7 lines

Now it complains about mode 8...
[    0.118835] genirq: Setting trigger mode 8 for irq 128 failed (gpio_irq_type
+0x0/0x220)
A.

On thread [1] was reported a regression on a Sitara AM335x board because the irq .map function handler used on the gpiolib called irq_set_type() for each pin on the bank.

In the case of that board, a GPIO pin is used to control the SDRAM termination regulator and calling irq_set_type() sets the GPIO as input thus making the board to fail on boot. But this may also explain why you had those error logs on your OMAP1 board when applying these patches since that changes GPIO OMAP semantics when the driver expected irq_set_type() to only be called when a IRQ is requested.

After some discussion Linus proposed a patch [2] which I think should also fix your issue. Can you please give it a try and provide your Tested-by for that patch?

Thanks a lot and best regards,
Javier

[1]: http://marc.info/?t=139817273800014&r=1&w=2
[2] https://patchwork.kernel.org/patch/4041881/