Re: [PATCH] gpio: Describe interrupt-controller binding

[Stephen Warren <swarren@wwwdotorg.org>]

On 09/18/2012 12:06 PM, Thierry Reding wrote:
> On Tue, Sep 18, 2012 at 08:55:40AM -0600, Stephen Warren wrote:
>> On 09/18/2012 07:28 AM, Rob Herring wrote:
>>> On 09/18/2012 03:51 AM, Thierry Reding wrote:
>>>> In order to use GPIO controllers as interrupt controllers,
>>>> they need to be marked with the DT interrupt-controller
>>>> property. This commit adds some documentation about this to
>>>> the general GPIO binding document.
>>>> 
>>>> Cc: Linus Walleij <linus.walleij@stericsson.com> Cc: Grant
>>>> Likely <grant.likely@secretlab.ca> Cc: Rob Herring
>>>> <rob.herring@calxeda.com> Cc:
>>>> devicetree-discuss@lists.ozlabs.org Cc:
>>>> linux-kernel@vger.kernel.org Signed-off-by: Thierry Reding
>>>> <thierry.reding@avionic-design.de>
>>> 
>>> Applied for 3.7.
>>> 
>>> Rob
>>> 
>>>> --- Documentation/devicetree/bindings/gpio/gpio.txt | 33
>>>> +++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt
>>>> b/Documentation/devicetree/bindings/gpio/gpio.txt index
>>>> 4e16ba4..8d125b0 100644 ---
>>>> a/Documentation/devicetree/bindings/gpio/gpio.txt +++
>>>> b/Documentation/devicetree/bindings/gpio/gpio.txt @@ -75,4
>>>> +75,37 @@ Example of two SOC GPIO banks defined as
>>>> gpio-controller nodes: gpio-controller; };
>>>> 
>>>> +If the GPIO controller supports the generation of
>>>> interrupts, it should +also contain an empty
>>>> "interrupt-controller" property as well as an 
>>>> +"#interrupt-cells" property. This is required in order for
>>>> other nodes +to use the GPIO controller as their interrupt
>>>> parent.
>> 
>> Surely this is generic information for any interrupt controller,
>> and hence doesn't belong in the GPIO binding?
> 
> LinusW requested this in order to avoid having to list these
> properties in every GPIO controller.

There's no need to list this property in an individual GPIO
controller's binding either; it's a standard property for any
interrupt controller of any type.

> I suppose that having it in an extra binding for interrupt
> controllers might make sense as well, but in that case we should
> probably provide a reference because the GPIO binding is where 
> people are most likely to look for this information.

Yes, we probably should have a centralized .txt for the base interrupt
controller properties. I guess we don't today because it's probably so
old everyone assumes it.

For some other patches I sent, I created
Documentation/devicetree/bindings/interrupt-controller/ - a file in
that directory would make sense (bike-shedding: irq.txt, interrupts.txt?)

Yes, each individual GPIO binding (that actually is an interrupt
controller; some may not be) should probably mention this and refer to
whatever documents the interrupt controller properties.

> There is Documentation/devicetree/bindings/open-pic.txt, which
> already lists most of this information, so maybe a reference to
> that document will do just as well?

I think that's just one random interrupt controller. The common/core
properties probably should be separated out.

>>>> +If #interrupt-cells is 1, the single cell is used to specify
>>>> the number +of the GPIO that is to be used as an interrupt. 
>>>> + +If #interrupt-cells is 2, the first cell is used to
>>>> specify the number +of the GPIO that is to be used as an
>>>> interrupt, whereas the second cell +is used to specify any of
>>>> the following flags: +  - bits[3:0] trigger type and level
>>>> flags +      1 = low-to-high edge triggered +      2 =
>>>> high-to-low edge triggered +      4 = active high
>>>> level-sensitive +      8 = active low level-sensitive
>> 
>> That certainly shouldn't be in the generic GPIO binding; the
>> format of the interrupt specifier is determined by the binding
>> for the individual device that is the interrupt controller. Just
>> because a device is also a GPIO controller doesn't mean that it
>> has to conform to a specific format for the interrupt specifier.
> 
> I think it does make sense to provide a description of the most
> commonly used variants. The above certainly is what the majority is
> using and many of those that do not use one of the predefined
> irq_domain_xlate_*() functions reimplement them with some
> additional checks or conversions.

OK, but the document can't say "this is how the IRQ specifier is
formatted", when it clearly isn't generally true.

The document should say that the format of the IRQ specifier is
entirely determined by the individual binding, but that bindings may
often choose to re-use the following common format. Each individual
binding would then need to document whether it did choose to use that
common format, or whether it instead chose something custom.