[U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors

[Marek Vasut]

> On Monday 12 December 2011 05:17:49 Marek Vasut wrote:
> > > On Sunday 11 December 2011 08:42:07 Marek Vasut wrote:
> > > > > On Saturday 10 December 2011 20:09:30 Marek Vasut wrote:
> > > > > > Introduce ne2k_register_io(in, out), which allows user to supply
> > > > > > two functions. One for reading data from the card, the other for
> > > > > > writing data to the card. Then introduce drivers' private data,
> > > > > > which carry pointers to these functions and are passed throughout
> > > > > > the driver.
> > > > > 
> > > > > where are the users of this new API ?  as it stands, i just see
> > > > > bloat. every register access is now an indirect function call ?
> > > > > what's the point
> > > > 
> > > > Go to ... drivers/net/ax88796.h ... and check how it's done now. It's
> > > > just wrong. Now for .03 release I have pxa3xx support ready which
> > > > uses just this chip and adding more sh^Htuff to that fill would be
> > > > even worse bloat.
> > > 
> > > i agree, that code is terrible.  however, those code paths can be
> > > trivially merged without the proposed bloat yours brings in.
> > 
> > So what's your suggested awesome clean solution?
> 
> there's no need to get snarky

Well sometimes it's hard to digest the style of your comments ;-)
> 
> rename ISA_OFFSET to CONFIG_NE2000_IO_OFFSET, then move the "2" to
> CONFIG_NE2000_IO_STRIDE, and move them both to the board config header.
> then you get one unified set:
> 
> #define DP_IN(_b_, _o_, _d_) \
> 	(_d_) = readw((void *)((_b_) + ((_o_) * CONFIG_NE2000_IO_STRIDE) + \
> 		CONFIG_NE2000_IO_OFFSET));
> etc...
> 
> if you really wanted to clean up the driver, the DP_XXX funcs would get
> turned into C code as static inline helpers, and the base + register
> offset would get turned into a C struct.

Ok, so if you had two different piece of hardware that had different 
NE2000_IO_OFFSET and STRIDE, running the same u-boot, how'd you handle it ?

> 
> > > further, that code base
> > > isn't even used by the ne2000 driver.
> > 
> > What are you talking about, did you even bother to look?
> 
> might want to cut the attitude.  it's really not adding anything.

Agreed
> 
> of course i looked and i saw ne2000.h defining DP_OUT/etc...  but i missed
> the ugly ifdef logic with CONFIG_DRIVER_AX88796L in ne2000_base.c.

That needs handling in a separate patch.
> 
> > > so again, the question stands: what exactly do you need to do different
> > > ? looks to me like the DP_* macros should get punted in favor of io.h
> > > accessors, and the register offsets rewritten into C structs.
> > 
> > Sure, but not every hardware accesses the registers the same way.
> 
> which is why we have asm/io.h in the first place.
> 
> on a semi-related note, these vu_{short,char,etc...} types need to get
> culled from the code base.  they're volatile markings in disguise ...

Agreed

M