Hi Doug,

On Wed, Oct 04, 2017 at 02:24:37PM -0700, Doug Berger wrote:

On 10/03/2017 08:03 PM, Gregory Fong wrote:

quoted

On Fri, Sep 29, 2017 at 8:40 PM, Doug Berger [off-list ref] wrote:

quoted

The GPIOLIB IRQ chip helpers were very appealing, but badly broke
the 1:1 mapping between a GPIO controller's device_node and its
interrupt domain.

Out of curiosity, what sort of problems have you seen from this?

[snip]

When another device-tree node references a GPIO device as its interrupt
parent, the irq_create_of_mapping() function looks for the irq domain of
the GPIO device and since all bank irq domains reference the same GPIO
device node it always resolves to the irq domain of the first bank
regardless of which bank the number of the GPIO should resolve. This
domain can only map hwirq numbers 0-31 so interrupts on GPIO above that
can't be mapped by the device-tree.

Thanks for clarifying.  This would be great information to include in
the commit message.

quoted

quoted

This commit consolidates the per bank irq domains to a version
where we have one larger interrupt domain per GPIO controller
instance spanning multiple GPIO banks.

This works (and is reminiscent to my initially submitted
implementation at [1]), but I think it might make sense to keep as-is
(using the gpiolib irqchip helpers), and instead allocate an irqchip
fwnode per bank and use to_of_node() to set it as the of_node for the
gpiochip before calling gpiochip_irqchip_add().  OTOH, that capability
might go away...

Linus, can you comment on the FIXME in gpiochip_irqchip_add_key() that
says "get rid of this and use gpiochip->parent->of_node everywhere"?
It seems like it would still be beneficial to be able to override the
associated node for a gpiochip, since that's what's used for the
irqdomain, but if that's going away, obviously we don't want to start
using that now.

Yes, this is effectively a reversion to an earlier implementation. I
produced an implementation based on the generic irqchip libraries, but
that was stripped from this submission when I discovered that no support
exists within the generic irqchip libraries for removal of domain
generic chips and we wanted to preserve the module support of this driver.

Considering this is heavily based on my initial implementation (several
functions are exactly the same), it'd be nice to have some small
attribution in the commit message. :-)

It is conceivable that the current GPIO device-tree nodes could be
broken down into separate devices per bank, but it is believed that this
would only confuse things for users of the device as the concept
diverges from the concept expressed in device documentation.

OK, that sounds like a worse alternative.  And since these are all
actually using the same parent IRQ, it does make sense to keep them all
in the same IRQ domain.


On Fri, Sep 29, 2017 at 08:40:56PM -0700, Doug Berger wrote:

Consider renaming "offset" to "hwirq".

Consider renaming "gc_offset" to "offset" and "offset" to "hwirq" to
keep things consistent.

I'm not a fan of the change to use "irq" to mean several different
things in this function.  How about this instead?

static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
{
	struct brcmstb_gpio_priv *priv = bank->parent_priv;
	struct irq_domain *domain = priv->irq_domain;
	int hwbase = bank->gc.base - priv->gpio_base;
	unsigned long status;

	while ((status = brcmstb_gpio_get_active_irqs(bank))) {
		int offset;

		for_each_set_bit(offset, &status, 32) {
			if (offset >= bank->width)
 				dev_warn(&priv->pdev->dev,
 					 "IRQ for invalid GPIO (bank=%d, offset=%d)\n",
					 bank->id, offset);
			generic_handle_irq(irq_linear_revmap(domain, offset + hwbase));

 		}
 	}
 }
[snip] 

The rest looks good.

Thanks,
Gregory