Message-Id: <36A77FB1-5EB3-4B2F-8E2B-5EDA4E097B7A@kernel.crashing.org>
Date:	Mon, 28 Oct 2013 01:54:34 -0500
From:	Kumar Gala <galak@...nel.crashing.org>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Rob Herring <robherring2@...il.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	Pawel Moll <Pawel.Moll@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>
Subject: Re: [RFC 9/9] of/irq: create interrupts-extended property


On Oct 27, 2013, at 10:16 PM, Mark Rutland wrote:

> On Sun, Oct 27, 2013 at 08:24:08PM +0000, Rob Herring wrote:
>> On Sun, Oct 27, 2013 at 8:46 AM, Grant Likely <grant.likely@...aro.org> wrote:
>>> On Tue, 15 Oct 2013 21:39:23 +0100, Grant Likely <grant.likely@...aro.org> wrote:
>>>> The standard interrupts property in device tree can only handle
>>>> interrupts coming from a single interrupt parent. If a device is wired
>>>> to multiple interrupt controllers, then it needs to be attached to a
>>>> node with an interrupt-map property to demux the interrupt specifiers
>>>> which is confusing. It would be a lot easier if there was a form of the
>>>> interrupts property that allows for a separate interrupt phandle for
>>>> each interrupt specifier.
>>>> 
>>>> This patch does exactly that by creating a new interrupts-extended
>>>> property which reuses the phandle+arguments pattern used by GPIOs and
>>>> other core bindings.
>>>> 
>>>> Signed-off-by: Grant Likely <grant.likely@...aro.org>
>>>> Cc: Rob Herring <rob.herring@...xeda.com>
>>> 
>>> Alright, I want to merge this one. I've got an Ack from Tony, general
>>> agreement from an in person converstaion from Ben (aside from wishing he
>>> could think of a better property name), and various rumblings of
>>> approval from anyone I talked to about it at ksummit. I'd like to have
>>> something more that that to put into the commit text. Please take a look
>>> and let me know if you agree/disagree with this binding.
> 
> The only comment I have is that I'm not keen on the name (it's a bit generic
> and we might want to extend the interrupts binding in future), but I'm happy to
> leave that as a matter of personal taste.
> 
> The best alternative I could come up with was parented-interrupts, but that
> could be misinterpreted as describing the relationship backwards (i.e. this
> device is the parent for those interrupts).

I think we seem to all agree the name sucks, but can't come up with anything better.

>> I think it looks fine, but I'll throw out an alternative proposal.
>> Simply allow for interrupt-parent to be an array in equal size to
>> interrupts property. Then it is a minimal change to the existing
>> binding:
>> 
>> interrupt-parent = <&intc1>, <&intc2>;
>> interrupts = <5 0>, <6 0>;
> 
> I'd prefer the interrupts-extended approach.
> 
> This might not matter, but if you have an existing driver with two interrupts,
> and you attempt to describe the situation this way, e.g.
> 
>  intc1: interrupt_controller1 {
>      compatible = "vendor,interrupt-controller";
>      #interrupt-cells = <1>;
>  };
> 
>  intc2: interrupt_controller2 {
>      compatible = "vendor,another-interrupt-controller";
>      #interrupt-cells = <2>;
>  };
> 
>  dev {
>      compatible = "vendor,some-device";
>      interrupt-parent = <&intc1>, <&intc2>;
>      interrupts = <5>, <65 73>;
>  };
> 
> The existing driver may read interrupts as two interrupts for intc1, and
> believe it's been successful when it has not, and one of those interrupts might
> never fire. That would be very bad for a timer for example.
> 
> Additionally it makes the interrupts list difficult for a human to read, as you
> need to match it with an entry in another list (this problem exists with other
> parallel properties like interrupt-names too, but we can't do much better
> there).
> 
> It's also easy to make a typo (e.g. an extra cell in an interrupt) that will
> parse as valid when you don't expect it to. At least with the phandle in the
> list it's less likely to happen.

I agreed, I'd assume that in Rob's suggestion the length of interrupt-parent & interrupts would have to match so you'd have:

 dev {
     compatible = "vendor,some-device";
     interrupt-parent = <&intc1>, <&intc2>, <&intc2>;
     interrupts = <5>, <65 73>;
 };

> 
>> 
>> Of course interrupts-extended is more inline with standard patterns
>> for bindings.
> 
> I think for this reason alone it should be preferable. Unless I'm mistaken it
> would be identical to the clock bindings pattern with a uniformly named
> phandle+args property and a parallel -names property. I don't think the gpio
> style ${NAME}-interrupts would easily fit here.
> 
> Thanks,
> Mark.

- k

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists