Re: [PATCH 2/2] checks: add interrupts property check

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

On Fri, Aug 18, 2017 at 10:53:56AM -0500, Rob Herring wrote:
> On Thu, Aug 17, 2017 at 11:43 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Aug 14, 2017 at 04:48:07PM -0500, Rob Herring wrote:
> >> Add a check for nodes with interrupts property that they have a valid
> >> parent, the parent has #interrupt-cells property, and the size is a
> >> valid multiple of #interrupt-cells.
> >>
> >> This may not handle every possible case and doesn't deal with
> >> translation thru interrupt-map properties, but should be enough for
> >> modern dts files.
> >>
> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> ---
> >>  checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 58 insertions(+)
> >>
> >> diff --git a/checks.c b/checks.c
> >> index c0450e118043..0d452bf8e674 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c,
> >>  }
> >>  WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
> >>
> >> +static void check_interrupts_property(struct check *c,
> >> +                                   struct dt_info *dti,
> >> +                                   struct node *node)
> >> +{
> >> +     struct node *root = dti->dt;
> >> +     struct node *irq_node = NULL, *parent = node;
> >> +     struct property *irq_prop, *prop = NULL;
> >> +     int irq_cells, phandle;
> >> +
> >> +     irq_prop = get_property(node, "interrupts");
> >> +     if (!irq_prop)
> >> +             return;
> >> +
> >> +     while (parent && !prop) {
> >> +             if (parent != node) {
> >
> > So, it's kind of academic, but is it actually disallowed for an
> > interrupt-controller node to itself have interrupts which are
> > implicityly routed to itself?
> 
> I think so, yes. Because otherwise how do you describe a chained
> interrupt controller? "interrupts" in the chained controller's node
> are the parent's interrupts. I guess you have to rely on the
> compatible implying any interrupts connected to itself. It could work
> with interrupts-extended though.

Ah.. yes.  My guess would be that you could do it by using an explicit
interrupt parent - which would mean that interrupt-parent would need
to take precedence over an interrupt controller in the same node.  I
guess we should probably check the old interrupt binding document.

Note that a possible, though strange case is for a node to have
interrupt-parent pointing at itself, with itself being an interrupt
nexus redirecting the interrupts elsewhere.  I've used this in the
past to handle a device with interrupts going to multiple interrupt
controllers before interrupts-extended was invented.

> 
> [...]
> 
> >> @@ -1103,6 +1160,7 @@ static struct check *check_table[] = {
> >>
> >>       &provider_cells_property,
> >>       &gpio_cells_property,
> >> +     &interrupts_property,
> >>
> >>       &always_fail,
> >>  };
> >
> > With both these patches testcases to make sure the checks actually
> > trip on a bad example would be good.
> 
> Yeah, I wanted to get some initial feedback first.

Fair enough.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]