On Sunday 11 November 2012, Viresh Kumar wrote:
> From: Shiraz Hashim <shiraz.has...@st.com>
>
> SPEAr3xx architecture includes shared/multiplexed irqs for certain set
> of devices. The multiplexor provides a single interrupt to parent
> interrupt controller (VIC) on behalf of a group of devices.
>
> There can be multiple groups available on SPEAr3xx variants but not
> exceeding 4. The number of devices in a group can differ, further they
> may share same set of status/mask registers spanning across different
> bit masks. Also in some cases the group may not have enable or other
> registers. This makes software little complex.
>
> Present implementation was non-DT and had few complex data structures to
> decipher banks, number of irqs supported, mask and registers involved.
>
> This patch simplifies the overall design and convert it in to DT. It
> also removes all registration from individual SoC files and bring them
> in to common shirq.c.
>
> Also updated the corresponding documentation for DT binding of shirq.
Looks basically ok, but I have a few comments.
> Signed-off-by: Shiraz Hashim <shiraz.has...@st.com>
> Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> ---
> .../devicetree/bindings/arm/spear/shirq.txt | 48 ++++
> arch/arm/mach-spear3xx/include/mach/irqs.h | 10 +-
> arch/arm/mach-spear3xx/spear300.c | 103 -------
> arch/arm/mach-spear3xx/spear310.c | 202 --------------
> arch/arm/mach-spear3xx/spear320.c | 204 --------------
> arch/arm/mach-spear3xx/spear3xx.c | 4 +
> arch/arm/plat-spear/include/plat/shirq.h | 35 +--
> arch/arm/plat-spear/shirq.c | 305
> +++++++++++++++++----
I guess it would be nice to move this to drivers/irqchip/st-shirq.c now
that we have introduced that directory.
> static const char * const spear320_dt_board_compat[] = {
> diff --git a/arch/arm/mach-spear3xx/spear3xx.c
> b/arch/arm/mach-spear3xx/spear3xx.c
> index 98144ba..781aec9 100644
> --- a/arch/arm/mach-spear3xx/spear3xx.c
> +++ b/arch/arm/mach-spear3xx/spear3xx.c
> @@ -121,6 +122,9 @@ struct sys_timer spear3xx_timer = {
>
> static const struct of_device_id vic_of_match[] __initconst = {
> { .compatible = "arm,pl190-vic", .data = vic_of_init, },
> + { .compatible = "st,spear300-shirq", .data = spear3xx_shirq_of_init, },
> + { .compatible = "st,spear310-shirq", .data = spear3xx_shirq_of_init, },
> + { .compatible = "st,spear320-shirq", .data = spear3xx_shirq_of_init, },
> { /* Sentinel */ }
> };
You list three "compatible" values here with the same init function, and then
> +int __init spear3xx_shirq_of_init(struct device_node *np,
> + struct device_node *parent)
> +{
> + struct spear_shirq **shirq_blocks;
> + void __iomem *base;
> + int block_nr, ret;
> +
> + base = of_iomap(np, 0);
> + if (!base) {
> + pr_err("%s: failed to map shirq registers\n", __func__);
> + return -ENXIO;
> + }
> +
> + if (of_device_is_compatible(np, "st,spear300-shirq")) {
> + shirq_blocks = spear300_shirq_blocks;
> + block_nr = ARRAY_SIZE(spear300_shirq_blocks);
> + } else if (of_device_is_compatible(np, "st,spear310-shirq")) {
> + shirq_blocks = spear310_shirq_blocks;
> + block_nr = ARRAY_SIZE(spear310_shirq_blocks);
> + } else if (of_device_is_compatible(np, "st,spear320-shirq")) {
> + shirq_blocks = spear320_shirq_blocks;
> + block_nr = ARRAY_SIZE(spear320_shirq_blocks);
> + } else {
> + pr_err("%s: unknown platform\n", __func__);
> + ret = -EINVAL;
> + goto unmap;
> + }
> +
> + ret = shirq_init(shirq_blocks, block_nr, base, np);
> + if (ret) {
> + pr_err("%s: shirq initialization failed\n", __func__);
> + goto unmap;
> + }
> +
> + return ret;
> +
> +unmap:
> + iounmap(base);
> + return ret;
> +}
In that multiplex between thre three again. I think it would be cleaner to have
three separate functions and move the call to of_iomap into shirq_init.
Arnd
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
