On Wednesday 09 January 2013, Mark Rutland wrote:
> Hi,
>
> Thanks for updating the text, this is far easier to read than previously.
>
> However, I'm still concerned by how complex the binding seems. As I don't have
> any familiarity with the device, I don't know whether that's just an artifact
> of the hardware or something that can be cleared up.
>
> I think the approach used by the binding needs some serious review before this
> should be merged. It seems far more complex than any existing interrupt
> controller binding. Without a dts example for a complete board (complete with
> devices wired up to the interrupt controller), it's difficult to judge how
> this
> will work in practice.
>
> I've added Arnd to Cc in case he has any thoughts on the matter.
Sorry for having been absent from this discussion for so long. I didn't
realize there were 9 versions of this patch set.
I tend to agree with your interpretation above, but I may be missing
important facts from the previous review rounds.
For all I can tell, the binding is an attempt to describe the
entire drivers/sh/intc capabilities, which is probably not the
best way to approach things. The sh intc driver is not just an
irqchip driver, but rather a framework to describe arbitrary
irqchips, which is what makes this so hard.
When I first looked at the situation last year, I suggested doing
a new irqchip driver with a much simpler binding that can only
handle the irq chips from shmobile, rather than the whole thing.
I am not sure if the binding in the current form is already the
"simplified" version, or if it actually implements all the
capabilities of the intc driver.
> > + intca: interrupt-controller@0 {
> > + compatible = "renesas,sh_intc";
> > + interrupt-controller;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + #interrupt-cells = <1>;
> > + ranges;
> > +
> > + reg = <0xe6940000 0x200>, <0xe6950000 0x200>;
> > + group_size = <19>;
> > +
> > + DIRC: intsrc1 { vector = <0x0560>; };
> > + ATAPI: intsrc2 { vector = <0x05E0>; };
>
> This looks suspiciously like a way of encoding a device's interrupt
> information
> into the interrupt controller's device node. That strikes me as being the
> wrong
> way round.
Agreed, it would be simpler to have e.g. #interrupt-cells = <4>, to describe
the various offsets when needed (I forgot how many are actually required
in practice, rather than being computable from the other numbers), and
possibly a global interrupt-map/interrupt-map-mask property pair to map
this into a flat number space.
I need to take some more time to understand the actual requirements again,
but IIRC it would be possible to do something much simpler than the
proposed binding.
Arnd
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
