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

[Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org>]

On 7/5/2012 4:54 AM, Arnd Bergmann wrote:
> 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.

The "mask-offset" property is really a "reg" in disguise.

"reg" is considerably more general than just "memory mapped
register address".  It really means "any numeric identifier
that makes sense in the context of a parent device".

Therefore, one could say:

gpio: gpio-controller {
          compatible = "marvell, orion-gpio";
          reg = <0xd0400 0x40>;
          #address-cells = <1>;
          #size-cells = <0>;
          ...

          bank0@0 {
                  reg = <0x0>;
                  ngpio = <8>;
                  mask-offset = <0>;
                  interrupts = <12>;
          };

          bank1@8 {
                  reg = <0x8>;
                  ngpio = <8>;
                  interrupts = <13>;
           };

>
>>
>> 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
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>