Re: [PATCH 1/8] SH: intc: Add support OF for INTC

[Simon Horman <horms@verge.net.au>]

On Thu, Jan 10, 2013 at 10:56:51AM +0900, Simon Horman wrote:
> On Wed, Jan 09, 2013 at 07:11:04PM +0000, Arnd Bergmann wrote:
> > 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.
> 
> I think its more on the side of implementing the capabilities of
> the intc driver than being simplified.
> 
> Although some effort has gone into this patchset my primary
> aim is to provide something that provides the basis for supporting
> the INTC controller on all existing boards.
> 
> I more than open to concrete ideas of how this can be achieved in agreeable way.
> 
> > > > +       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'm not sure that I see what you are getting at here.
> 
> > 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.

Ping