From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Frysinger Date: Fri, 16 Dec 2011 12:33:53 -0500 Subject: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors In-Reply-To: <201112121117.49932.marek.vasut@gmail.com> References: <1323565772-4498-1-git-send-email-marek.vasut@gmail.com> <201112120045.39688.vapier@gentoo.org> <201112121117.49932.marek.vasut@gmail.com> Message-ID: <201112161233.54351.vapier@gentoo.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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. > > 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. 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. > > 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 ... -mike -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: This is a digitally signed message part. URL: