Re: [PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head

On Thursday 12 June 2008 14:12:15 you wrote:

On Jun 12, 2008, at 6:45 AM, David Jander wrote:

Your commit message isn't exactly helpful as most people dont know
what LTIB is and its not terribly relevant.  It just seems like you
are adding support for the FEC on MPC5121 and this point.

[...]

quoted

--- a/drivers/net/fec.h
+++ b/drivers/net/fec.h

@@ -59,6 +59,7 @@ typedef struct fec {

} fec_t;

#else
+#if !defined(CONFIG_FS_ENET_MPC5121_FEC)

/*
 *	Define device register set address map.

@@ -97,6 +98,48 @@ typedef struct fec {

	unsigned long	fec_fifo_ram[112];	/* FIFO RAM buffer */
} fec_t;

+#else /* CONFIG_FS_ENET_MPC5121_FEC */
+
+typedef struct fec {
[...]
+} fec_t;
+
+#endif /* CONFIG_FS_ENET_MPC5121_FEC */
#endif /* CONFIG_M5272 */

I'm not exactly clear as to why this was done this way but this not
acceptable as it means we can't build a multiplatform kernel that
needs this driver.

Well, it wouldn't be possible either, since CONFIG_M5272 is a Cold-fire 
processor, and CONFIG_FS_ENET_MPC5121_FEC is for a PowerPC processor.
In this case.
Otherwise you are right, the driver breaks MPC83xx/MPC5121 multiplatform 
builds.

I'm also not clear to me if the MPC5121 FEC is really the same device
or close to it that it should be sharing this driver or have its own.

I am coming to the conclusion that it should have its own driver. 
Altough a lot of code could be shared, there are still enough differences, so 
that writing just ONE driver without some #ifdef's that would break 
multiplatform builds, would instead end up with a much bigger amount of if's, 
that would make it unreadable, unmaintainable and inefficient.
Here's why: The above struct fet_t for instance is mapped to a set of 
registers in the FEC. For processors with a CPM1, a CPM2 or without CPM (i.e. 
MPC5121) the register mapping seems to be significantly different, 
nevertheless the structs are all called "struct fec_t". How can one fix this 
at runtime without changing the name of the structs and then just use a lot 
of "if's" or a combination of macro's and if's everywhere a register of the 
FEC is accessed? I fear it will be a mess.

So I think it's either a separate driver, or break multiplatform builds.

Since I am learning from you that breaking multiplatform builds is a no-go, 
I'll settle for splitting up the driver.

Any suggestion on where to put that split-off? How to name it?
I would suggest drivers/net/fec_mpc512x/*

I just resubmitted PATCH 1/2 again without part 2 (which hasn't much to do 
with it anyway), so Grant may have a final look at it (hopefully I did it 
right this time). Part 2 (MPC5121_FEC) will have to wait until monday or so, 
since it will take me a while, and I have to do other things in between.

Any suggestions on how to solve the puzzle are of course welcome...

Thanks a lot for reviewing.

Best regards,

-- 
David Jander
Protonic Holland.