On Mon, 11 Dec 2006 16:08:20 -0600
Kumar Gala [off-list ref] wrote:

On Dec 11, 2006, at 3:51 PM, Kim Phillips wrote:

quoted

On Sun, 10 Dec 2006 21:41:24 -0600
Kumar Gala [off-list ref] wrote:

quoted

On Dec 9, 2006, at 1:14 AM, Benjamin Herrenschmidt wrote:

quoted

On Fri, 2006-12-08 at 19:07 -0600, Kim Phillips wrote:

quoted

Eliminate code redundancy.  mpc83[246]x_{mds,itx,sys,pb} files  
merged
into a single setup.c.  machine_probe, instead of using the model
property,
checks the compatible property for "MPC83xx" (dts files updated
appropriately).
This patch also utilizes of_platform_bus_probe() in lieu of  
manually
calling of_platform_device_create for each ucc_geth device.

Signed-off-by: Kim Phillips <redacted>

Nack on the patch.

quoted

I am not completely certain this is the right approach.

While factoring code is good, I think that every single board should
have it's own ppc_md, though you can definitely provide "common"
functions for mpc83xx that can optinally be used by those different
boards.

Maybe put all the freescale ones in one file if you want...

The rationale here is that while your approach is fine for your eval
boards, I don't think it's good for embedded customers. They may  
want
more complex platforms, with their own directory even if they have
a lot
of custom stuff on the board while still possibly picking some of  
your
"common" code (and their board shouldn't match your overly generic
probe() implementation).

Cheers,
Ben.

I'm with Ben on this.  I think consolidating the code that is common
is fine, but we should have a define_machine() per board.  You can
put them all in one mpc83xx/fsl.c

so the contents of 83xx/fsl.c would look like:
#ifdef CONFIG_MPC834x_SYS
define_machine(mpc834x_sys) {
	.name 		= "MPC834x SYS",
	.probe 		= mpc83xx_probe,
	.setup_arch 	= mpc83xx_setup_arch,
	.init_IRQ 	= mpc83xx_init_IRQ,
	.get_irq 	= ipic_get_irq,
	.restart 	= mpc83xx_restart,
	.time_init 	= mpc83xx_time_init,
	.calibrate_decr	= generic_calibrate_decr,
	.progress 	= udbg_progress,
};
#else
#ifdef CONFIG_MPC834x_ITX
define_machine(mpc83xx) {
	.name 		= "MPC834x ITX",
<rest is the same>
#else
..and so on and so forth for CONFIG_MPC8360E_PB,  
CONFIG_MPC832x_MDS, and now CONFIG_MPC831x_RDB.  So the only thing  
that changes is the name?  And perhaps the _probe function?  That's  
still pretty redundant, esp. considering the source of the name to  
match in the dt is easily modifiable.  And it's not as if other  
platforms (52xx, 86xx, pasemi etc.) don't use  
of_flat_dt_is_compatible.

Why would you need the ifdef's around the define_machine()?

how else is platform_probe going to find the right match?  it's either that, or adding _probe()s for each platform.

quoted

All 83xx platform code, define_machine included, is exactly the  
same on all boards, modulo the QE stuff which is ifdef protected  
and easily configured.  All board differences can be specified in  
the device tree and/or handled with proper kernel configuration.   
If an embedded customer wants, they can configure their kernel  
based on the various 83xx defconfigs, customize their _probe() to  
match their device tree, all at will, just as before.  I'm  
concerned because 83xx has the capability of being a single kernel  
image basically for free (we just need to clean some things up in  
the ucc_geth driver first).

You say this, but miss the point that its only true for ALL FREESCALE  
boards, which are the only boards currently in the tree.  We aren't  

it's not my fault that only fsl boards are the only ones in the tree.  And why is it so impossible that other boards are the same way?

precluding the ability to have a single kernel image for 83xx.

good to know.

And I'm suggesting that for 831x_rdb, All you'd add is a new  
define_machine() struct.

how about making a single _probe() try and match the ppc_md.name with the model in the device tree?

Kim