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