Re: [PATCH 02/12] fs_enet: Add MPC5121 FEC support.

[John Rigby <jcrigby@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

Wolfgang,

Welcome to my world and why I gave up on this months ago.

Everyone else,

One thing to consider here is a rewrite with the goal of a new improved fec
driver that would work on both 5121 and the various mx platforms that also
have this same fec core.

Also don't forget that the register map is the same on 512x, mx and coldfire
platforms but not on the other ppc platforms so if you want to one binary to
rule them all you will need to have an offest table or some such.

John

On Thu, May 7, 2009 at 8:09 AM, Grant Likely <grant.likely@secretlab.ca>wrote:

> On Wed, May 6, 2009 at 4:41 PM, Wolfgang Denk <wd@denx.de> wrote:
> > Dear Grant,
> >
> > In message <fa686aa40905061529u11b231afle3b5bb10a2334ad0@mail.gmail.com>
> you wrote:
> >>
> >> > Agreed that it's ugly, but duplicatio9ng the code would have been even
> >> > worse. I don't think that it has multiplatform - at least not as long
> >> > as you don't ask for one image that runs on 83xx and on 512x.
> >>
> >> Actually, I *am* asking for one image that runs on 83xx, 52xx and
> >> 521x.  I already can and do build and test a single image which boots
> >> on all my 52xx boards, on my 8349 board, and on my G4 Mac.
> >
> > He. I was afraid you'd say that ;-)
> >
> > In this case I need a helping hand as I can't figure out how to make
> > this work. Any suggestions?
>
> Hmmm, it is rather ugly because the layout of fec_t is so different.
> Easiest solution would be to duplicate the driver in its entirety, but
> as you say it results in a lot of duplicate code.  It might be the
> lesser of many weevils though.
>
> Second easiest would be to factor out all the common code in the
> driver into a separate .c file that gets included by a 'wrapper' .c
> file for each config which first includes the correct definition of
> fec_t.  This approach solves the duplicate code problem, but it also
> fell out of the ugly tree and hit every branch on the way down.
>
> ie: the wrappers would look something like this:
>
> fs_enet_cpm1.c:
> #include <asm/cpm1.h>
> #include "fs_enet_main.c"
>
> fs_enet_cpm2.c:
> #include <asm/cpm2.h>
> #include "fs_enet_main.c"
>
> fs_enet_512x.c:
> #include <asm/mpc512x.h>
> #include "fs_enet_main.c"
>
> And then the makefile would be something along the lines of:
> obj-${CONFIG_FS_ENET_CPM1_ += fs_enet_cpm1.o
> obj-${CONFIG_FS_ENET_CPM2_ += fs_enet_cpm2.o
> obj-${CONFIG_FS_ENET_MPC512x_ += fs_enet_512x.o
>
> A brief look at the driver suggests that access to the fec_t structure
> is restricted to the fec-mii.c and mac-fec.c files.  It might be
> appropriate to duplicate just those files and do some form of
> fs_enet_ops to select between them.
>
> While on the topic, it looks to me like the driver could really use
> some refactoring love.  Having multiple definitions of "fec_t" is
> confusing and potentially lead to hard to find bugs if the wrong
> header gets included by anyone.  I'd like to see all the fec specific
> stuff in arch/powerpc/include/asm moved into drivers/net/fs_enet and
> renamed to reflect the hardware it is associate with.  Stuff like
> "fec_t" is far to generic, not to mention that typedefs are
> discouraged now.
>
> Regardless, I feel your pain.  It is not a pretty situation.
>
> g.
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>

[-- Attachment #2: Type: text/html, Size: 4196 bytes --]