Hi Prabhakar Lad,

On 11/02/2013 05:39 PM, Lad, Prabhakar wrote:

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

This patch converts the davinci gpio driver to use irqdomain
support.

This patch needs to be splitted in two:
1) add IRQ domain support
2) remove intc_irq_num

Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
  arch/arm/mach-davinci/da830.c              |    1 -
  arch/arm/mach-davinci/da850.c              |    1 -
  arch/arm/mach-davinci/dm355.c              |    1 -
  arch/arm/mach-davinci/dm365.c              |    1 -
  arch/arm/mach-davinci/dm644x.c             |    1 -
  arch/arm/mach-davinci/dm646x.c             |    1 -
  drivers/gpio/gpio-davinci.c                |   49 ++++++++++++++++++----------
  include/linux/platform_data/gpio-davinci.h |    3 +-
  8 files changed, 32 insertions(+), 26 deletions(-)

[...]

Sorry, but I don't understand why have you used <0> as hwirq?

All below logic may not work in case if we switch to use Linear IRQ domain :(
- irq_create_mapping() may return ANY Linux IRQ number. It means, for 
example, for bank0(ngpio=32)[0] it may return Linux_IRQ=200 or 201 or any other.
 Also, for bank3(ngpio=16)[0] it may return Linux_IRQ=150, etc.
- More over, if you call irq_create_mapping() 32 times you may NOT get
 sequential Linux_IRQ numbers.

So, the better sequence here can be smth. as below
(I can't verify it - my HW support only unbanked IRQs):

	if (irq & 1)
		mask <<= 16;

	while (1) {
		u32		status;
		int		bit;

		/* ack any irqs */
		status = __raw_readl(&g->intstat) & mask;
		if (!status)
			break;
		__raw_writel(status, &g->intstat);

		/* now demux them to the right lowlevel handler */
		while (status) {
			bit = __ffs(status);
                        status &= ~(1 << bit);
			generic_handle_irq(irq_find_mapping(d->irq_domain, bit));
		}
	}

I think you can use irq_create_mapping() here instead of 
irq_find_mapping(), so IRQ will be mapped/allocated on demand.
Also, it seems, above code may crash in case if SoC has >1 GPIO banks and
gpio_unbanked > 0 and someone will call gpio_to_irq(>31).

Use chips[i].chip.ngpio here instead of const 32?

+						      &irq_domain_simple_ops,

Pass &davinci_gpio_irq_ops (see below)

+						      NULL);

Pass &chips[bank] as host_data and use .map() callback (see below)

As I mentioned above, I think, it is not good to play with IRQ numbers here.
Only chained IRQs and <binten> can be configured in this cycle.

+
+		for (i = 0; i < 16 && gpio < ngpio; i++, gpio++) {
+			int irqno =
+				irq_create_mapping(chips[gpio / 32].irq_domain,
+						   i + irq);
+
+			irq_set_chip(irqno, &gpio_irqchip);
+			irq_set_chip_data(irqno, (__force void *)g);
+			irq_set_handler_data(irqno, (void *)__gpio_mask(gpio));
+			irq_set_handler(irqno, handle_simple_irq);
+			set_irq_flags(irqno, IRQF_VALID);

It makes no sense to manually create mapping. Usually it can be done in
.map() callback of IRQ domain. Like:

static int davinci_gpio_irq_map(struct irq_domain *d, unsigned int irq,
			    irq_hw_number_t hw)
{
	struct davinci_gpio_controller *chip = d->host_data;
	unsigned gpio = chip->chip.base + hw;

	irq_set_chip_and_handler_name(irq, &gpio_irqchip, handle_simple_irq,
				      "davinci_gpio");
	irq_set_irq_type(irq, IRQ_TYPE_NONE);
	set_irq_flags(irq, IRQF_VALID);
...
	return 0;
}

static const struct irq_domain_ops davinci_gpio_irq_ops = {
	.map = davinci_gpio_irq_map,
	.xlate = irq_domain_xlate_onetwocell,
};

[...]

  	spinlock_t		lock;
  	void __iomem		*regs;

start

Regards,
- grygorii