Re: [PATCH] General CHRP/MPC5K2 platform support patch

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

Any reason why this is not static nor __init ? :
> +struct device_node *find_mpc52xx_pic(void)
> +{
> +	struct device_node *dev;
> +	const char *piccompatible_list[] =
> +	{
> +		"mpc5200-interrupt-controller",
> +		"mpc52xx-interrupt-controller",
> +		"mpc52xx-pic",
> +		"mpc5200-pic",
> +		"5200-interrupt-controller",
> +		"52xx-interrupt-controller",
> +		"52xx-pic",
> +		"5200-pic",
> +		NULL
> +	};
> +
> +	/* Look for an MPC52xx interrupt controller */
> +	for_each_node_by_type(dev, "interrupt-controller")
> +	{
> +		const char **piccompatible_entry = piccompatible_list;
> +
> +		for(piccompatible_entry = piccompatible_list; *piccompatible_entry; piccompatible_entry++ )
> +		{
> +			if (device_is_compatible(dev, *piccompatible_entry ))
> +				return dev;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static int  __init chrp_find_mpc52xx_pic(void)
> +{
> +	if (find_mpc52xx_pic())
> +	{
> +		printk(KERN_INFO "Found MPC52xx Interrupt Controller\n");
> +		ppc_md.get_irq = mpc52xx_get_irq;
> +		mpc52xx_init_irq();
> +		return 0;
> +	}
> +
> +	return -ENODEV;
> +}

I'm not sure I see any point in splitting in two functions like that
 
>  static void __init chrp_find_8259(void)
>  {
>  	struct device_node *np, *pic = NULL;
> @@ -473,8 +519,11 @@ static void __init chrp_find_8259(void)
>  		break;
>  	}
>  	if (np == NULL)
> -		printk(KERN_WARNING "Cannot find PCI interrupt acknowledge"
> -		       " address, polling\n");
> +	{
> +		printk(KERN_WARNING "Cannot find PCI/i8259 interrupt acknowledge"
> +		       " Fix your tree!\n");
> +		return;
> +	}

Unrelated changes/fix, should probably be in a separate patch.

Cheers,
Ben.