Re: [PATCH v3 1/7] ARM: davinci, intc: Add OF support for TI interrupt controller

[Heiko Schocher <hs@xxxxxxx>]

Hello Grant,

Am 26.05.2012 03:05, schrieb Grant Likely:
> On Mon,  5 Mar 2012 12:09:58 +0100, Heiko Schocher<hs@xxxxxxx>  wrote:
> > Add a function to initialize the Common Platform Interrupt Controller
> > (cp_intc) from TI used on OMAP-L1x SoCs using a device tree node.
> >
> > Signed-off-by: Heiko Schocher<hs@xxxxxxx>
> > Cc: davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Cc: devicetree-discuss@xxxxxxxxxxxxxxxx
> > Cc: Grant Likely<grant.likely@xxxxxxxxxxxx>
> > Cc: Sekhar Nori<nsekhar@xxxxxx>
> > Cc: Wolfgang Denk<wd@xxxxxxx>
> > Cc: Sergei Shtylyov<sshtylyov@xxxxxxxxxx>
> >
> > ---
> > - changes for v2:
> > - add comment from Grant Likely:
> >    - migrate the whole interrupt controller to natively use an
> >      irq_domain. Rebased complete patchserie to:
> >      git://git.secretlab.ca/git/linux-2.6.git irqdomain/next
> >
> >      commit 3a806bfcde2cc3e4853f2807b2e3c94e7ccaf450
> >      Author: Grant Likely<grant.likely@xxxxxxxxxxxx>
> >      Date:   Fri Jan 27 06:44:34 2012 -0700
> >
> >      irq_domain: mostly eliminate slow-path revmap lookups
> > - changes for v3:
> >    - add comments from Sergei Shtylyov:
> >      - rename compatible" prop to "ti,cp_intc"
>
> Nit: DT preference is to use '-' instead of '_'

fixed.

[...]
> > diff --git a/Documentation/devicetree/bindings/arm/davinci/intc.txt b/Documentation/devicetree/bindings/arm/davinci/intc.txt
> > new file mode 100644
> > index 0000000..dfd6a560
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/davinci/intc.txt
> > @@ -0,0 +1,27 @@
[...]
> > +- compatible : should be:
> > +	"ti,cp_intc"
> > +- interrupt-controller : Identifies the node as an interrupt controller
> > +- #interrupt-cells : Specifies the number of cells needed to encode an
> > +  interrupt source. The type shall be a<u32>  and the value shall be 1.
>
> Is there any configuration for irq inputs (edge/level) or are they
> fixed mode?  If configurable, then you'll want another cell for flags.

As I know, they are fixed...

[...]
> > diff --git a/arch/arm/mach-davinci/cp_intc.c b/arch/arm/mach-davinci/cp_intc.c
> > index f83152d..585114a 100644
> > --- a/arch/arm/mach-davinci/cp_intc.c
> > +++ b/arch/arm/mach-davinci/cp_intc.c
> > @@ -9,9 +9,14 @@
[...]
> > +		/* create a legacy host */
> > +		cp_intc_domain = irq_domain_add_legacy(node, num_irq,
> > +					irq_base, 0,&cp_intc_host_ops, NULL);
> > +		if (cp_intc_domain == NULL) {
> > +			pr_err("CP INTC: failed to allocate irq host!\n");
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		/* Set up genirq dispatching for cp_intc */
> > +		for (i = 0; i<  num_irq; i++) {
> > +			irq_set_chip(i,&cp_intc_irq_chip);
> > +			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
> > +			irq_set_handler(i, handle_edge_irq);
> > +		}
>
> Instead of the else clause here, even the non-DT mode should still
> call irq_domain_add.  irq_domain is not just for DT.  It gets the
> driver away from manually managing its irq mappings.

Ok, I try this.

> >   	/* Enable global interrupt */
> >   	cp_intc_write(1, CP_INTC_GLOBAL_ENABLE);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static struct of_device_id irq_match[] __initdata = {
> > +	{ .compatible = "ti,cp_intc", .data = __cp_intc_init, },
> > +	{ }
> > +};
> > +
> > +void __init cp_intc_init(void)
> > +{
> > +	of_irq_init(irq_match);
> > +}
> > +#else
> > +void __init cp_intc_init(void)
> > +{
> > +	__cp_intc_init(NULL, NULL);
> >   }
> > +#endif
>
> Nack on this hunk because it means you have either a DT kernel or a
> non-DT kernel.  Turning on CONFIG_OF must not break booting on a
> non-DT platform.  This is a policy decision to improve test coverage.
> Instead the init hook should check at runtime if a DT is available,
> and if it is call of_irq_init.  Otherwise call __cp_initc_init()
> directly.

fixed in v4

Thanks for the review!

bye,
Heiko

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel