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

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 5769 bytes --]

On Tue, Sep 18, 2012 at 12:15:02PM -0600, Stephen Warren wrote:
> 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.

Since each driver binding still needs to document it and in order to
avoid needless duplication I think having a central location for this
makes a lot of sense.

> 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.

Okay. So how about I add a file interrupts.txt in that directory and put
something like what this patch contains into it? Then I can just add a
reference to the driver binding that the controller can be used as an
interrupt-controller and that a description can be find in this new
document.

> > 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.

Yes, that makes sense.

Rob, given the above discussion I think it'd be better if I followed up
with a patch that moves this description into a more generic location
and we removed this patch.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]