Re: [PATCH 1/2] qe/ic: move qe_ic_init from platforms to irqchip

[Jason Cooper <jason@lakedaemon.net>]

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@nxp.com>
> ---
>  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.