Re: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip
From: Jason Cooper
Date: Mon Jul 04 2016 - 23:18:51 EST
Hi Zhao Qiang,
Please reword your subject line to conform to the standard in irqchip
(since that's where the code is added). Also, please adjust the subject
line to express *why* the change is being made.
On Tue, Jul 05, 2016 at 09:46:58AM +0800, Zhao Qiang wrote:
> The codes of qe_ic_init in platforms are redundant,
> move them to qe_ic under irqchip
This needs to be a lot more clear. How is backwards compatibility
preserved? Why is lookup by type "qeic" dropped? In short, please
explain everything that looks funny so we don't have to guess.
> Signed-off-by: Zhao Qiang <qiang.zhao@xxxxxxx>
> ---
> arch/powerpc/platforms/83xx/misc.c | 15 ---------------
> arch/powerpc/platforms/85xx/corenet_generic.c | 9 ---------
> arch/powerpc/platforms/85xx/mpc85xx_mds.c | 14 --------------
> arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 16 ----------------
> arch/powerpc/platforms/85xx/twr_p102x.c | 14 --------------
> drivers/irqchip/qe_ic.c | 14 ++++++++++++++
> 6 files changed, 14 insertions(+), 68 deletions(-)
>
> diff --git a/arch/powerpc/platforms/83xx/misc.c b/arch/powerpc/platforms/83xx/misc.c
> index 7e923ca..9431fc7 100644
> --- a/arch/powerpc/platforms/83xx/misc.c
> +++ b/arch/powerpc/platforms/83xx/misc.c
> @@ -93,24 +93,9 @@ void __init mpc83xx_ipic_init_IRQ(void)
> }
>
> #ifdef CONFIG_QUICC_ENGINE
> -void __init mpc83xx_qe_init_IRQ(void)
> -{
> - struct device_node *np;
> -
> - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
> - if (!np) {
> - np = of_find_node_by_type(NULL, "qeic");
> - if (!np)
> - return;
> - }
This block isn't preserved in the irqchip driver. Why not?
> - qe_ic_init(np, 0, qe_ic_cascade_low_ipic, qe_ic_cascade_high_ipic);
> - of_node_put(np);
> -}
> -
> void __init mpc83xx_ipic_and_qe_init_IRQ(void)
> {
> mpc83xx_ipic_init_IRQ();
> - mpc83xx_qe_init_IRQ();
> }
> #endif /* CONFIG_QUICC_ENGINE */
>
> diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c
> index a2b0bc8..526fc2b 100644
> --- a/arch/powerpc/platforms/85xx/corenet_generic.c
> +++ b/arch/powerpc/platforms/85xx/corenet_generic.c
> @@ -41,8 +41,6 @@ void __init corenet_gen_pic_init(void)
> unsigned int flags = MPIC_BIG_ENDIAN | MPIC_SINGLE_DEST_CPU |
> MPIC_NO_RESET;
>
> - struct device_node *np;
> -
> if (ppc_md.get_irq == mpic_get_coreint_irq)
> flags |= MPIC_ENABLE_COREINT;
>
> @@ -50,13 +48,6 @@ void __init corenet_gen_pic_init(void)
> BUG_ON(mpic == NULL);
>
> mpic_init(mpic);
> -
> - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
> - if (np) {
> - qe_ic_init(np, 0, qe_ic_cascade_low_mpic,
> - qe_ic_cascade_high_mpic);
> - of_node_put(np);
> - }
> }
>
> /*
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> index f61cbe2..7ae4901 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> @@ -279,20 +279,6 @@ static void __init mpc85xx_mds_qeic_init(void)
> of_node_put(np);
> return;
> }
> -
> - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
> - if (!np) {
> - np = of_find_node_by_type(NULL, "qeic");
> - if (!np)
> - return;
> - }
> -
> - if (machine_is(p1021_mds))
> - qe_ic_init(np, 0, qe_ic_cascade_low_mpic,
> - qe_ic_cascade_high_mpic);
> - else
> - qe_ic_init(np, 0, qe_ic_cascade_muxed_mpic, NULL);
This block is also not preserved. Nor explained in the commit log. Is
it really ok to drop it? If so, why?
> - of_node_put(np);
> }
> #else
> static void __init mpc85xx_mds_qe_init(void) { }
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> index 3f4dad1..779f54f 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_rdb.c
> @@ -49,10 +49,6 @@ void __init mpc85xx_rdb_pic_init(void)
> struct mpic *mpic;
> unsigned long root = of_get_flat_dt_root();
>
> -#ifdef CONFIG_QUICC_ENGINE
> - struct device_node *np;
> -#endif
> -
> if (of_flat_dt_is_compatible(root, "fsl,MPC85XXRDB-CAMP")) {
> mpic = mpic_alloc(NULL, 0, MPIC_NO_RESET |
> MPIC_BIG_ENDIAN |
> @@ -67,18 +63,6 @@ void __init mpc85xx_rdb_pic_init(void)
>
> BUG_ON(mpic == NULL);
> mpic_init(mpic);
> -
> -#ifdef CONFIG_QUICC_ENGINE
> - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
> - if (np) {
> - qe_ic_init(np, 0, qe_ic_cascade_low_mpic,
> - qe_ic_cascade_high_mpic);
> - of_node_put(np);
> -
> - } else
> - pr_err("%s: Could not find qe-ic node\n", __func__);
> -#endif
> -
> }
>
> /*
> diff --git a/arch/powerpc/platforms/85xx/twr_p102x.c b/arch/powerpc/platforms/85xx/twr_p102x.c
> index 71bc255..603e244 100644
> --- a/arch/powerpc/platforms/85xx/twr_p102x.c
> +++ b/arch/powerpc/platforms/85xx/twr_p102x.c
> @@ -35,26 +35,12 @@ static void __init twr_p1025_pic_init(void)
> {
> struct mpic *mpic;
>
> -#ifdef CONFIG_QUICC_ENGINE
> - struct device_node *np;
> -#endif
> -
> mpic = mpic_alloc(NULL, 0, MPIC_BIG_ENDIAN |
> MPIC_SINGLE_DEST_CPU,
> 0, 256, " OpenPIC ");
>
> BUG_ON(mpic == NULL);
> mpic_init(mpic);
> -
> -#ifdef CONFIG_QUICC_ENGINE
> - np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
> - if (np) {
> - qe_ic_init(np, 0, qe_ic_cascade_low_mpic,
> - qe_ic_cascade_high_mpic);
> - of_node_put(np);
> - } else
> - pr_err("Could not find qe-ic node\n");
> -#endif
> }
>
> /* ************************************************************************
> diff --git a/drivers/irqchip/qe_ic.c b/drivers/irqchip/qe_ic.c
> index ec2ca86..f7f9a81 100644
> --- a/drivers/irqchip/qe_ic.c
> +++ b/drivers/irqchip/qe_ic.c
> @@ -509,4 +509,18 @@ static int __init init_qe_ic_sysfs(void)
> return 0;
> }
>
> +static int __init qeic_of_init(void)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
> + if (np) {
> + qe_ic_init(np, 0, qe_ic_cascade_low_mpic,
> + qe_ic_cascade_high_mpic);
> + of_node_put(np);
> + }
> + return 0;
> +}
> +
> +subsys_initcall(qeic_of_init);
> subsys_initcall(init_qe_ic_sysfs);
Was this tested on systems with dtbs containing type "qeic" but missing
"fsl,qe-ic"? What about non-p1021_mds mpc85xx_mds boards?
thx,
Jason.