Re: [PATCH v2 01/12] ARM: Orion: DT support for IRQ and GPIO Controllers

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Thursday 05 July 2012, Sebastian Hesselbarth wrote:
> Andrew,
> 
> is it possible to group all gpio banks into one DT description?
> For mach-dove it could be something like:
> 
> gpio: gpio-controller {
>         compatible = "marvell, orion-gpio";
>         ...
> 
>         bank0@d0400 {
>                 reg = <0xd0400 0x40>;
>                 ngpio = <8>;
>                 mask-offset = <0>;
>                 interrupts = <12>;
>         };
> 
>         bank1@d0400 {
>                 reg = <0xd0400 0x40>;
>                 ngpio = <8>;
>                 mask-offset = <8>;
>                 interrupts = <13>;
>         };

This way you have multiple nodes with the same register
and different names, which is not how it normally works.

> 
> This would have the advantage that DT describes gpio-to-irq dependencies.
> Moreover, nodes that reference gpios can do gpios = <&gpio 71 0>; instead of
> gpios = <&gpio3 7 0>;

Is that desired?

The device tree representation should match what is in the data sheet
normally. If they are in a single continuous number range, then we should
probably have a single device node with multiple register ranges
rather than one device node for each 32-bit register. Looking at
arch/arm/plat-orion/gpio.c I think that is not actually the case though
and having separate banks is more logical.

Something completely different I just noticed in the original patch:

> @@ -90,6 +74,27 @@ static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc)
>       }
>  }
>  
> +static int __initdata gpio0_irqs[4] = {
> +     IRQ_DOVE_GPIO_0_7,
> +     IRQ_DOVE_GPIO_8_15,
> +     IRQ_DOVE_GPIO_16_23,
> +     IRQ_DOVE_GPIO_24_31,
> +};
> +
> +static int __initdata gpio1_irqs[4] = {
> +     IRQ_DOVE_HIGH_GPIO,
> +     0,
> +     0,
> +     0,
> +};

I think the latter one needs to be

+static int __initdata gpio1_irqs[4] = {
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+};

so we register all four parts to the same primary IRQ. The
same is true for the devicetree representation.

	Arnd