From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Warren Date: Thu, 27 Mar 2008 09:56:48 -0400 Subject: [U-Boot-Users] [PATCH v2 4/7] add SMSC LAN9x1x Network driver In-Reply-To: <000101c88ff6$dbd44eb0$3a4d010a@Emea.Arm.com> References: <000101c88ff6$dbd44eb0$3a4d010a@Emea.Arm.com> Message-ID: <47EBA7A0.5010405@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Peter, Peter Pearse wrote: >> -----Original Message----- >> From: Ben Warren [mailto:biggerbadderben at gmail.com] >> Sent: 26 March 2008 20:08 >> To: Guennadi Liakhovetski >> Cc: u-boot-users at lists.sourceforge.net; Wolfgang Denk; Peter Pearse >> Subject: Re: [U-Boot-Users] [PATCH v2 4/7] add SMSC LAN9x1x >> Network driver >> >> Hi Guennadi, >> >> Guennadi Liakhovetski wrote: >> >>> From: Sascha Hauer >>> >>> This patch adds a driver for the following smsc network controllers: >>> LAN9115 >>> LAN9116 >>> LAN9117 >>> LAN9215 >>> LAN9216 >>> LAN9217 >>> >>> >>> >> How many of these have been tested, and on what platforms. >> I'm asking because the code seems to assume a 32-bit >> interface and these aren't all 32-bit chips. >> > > Comments please Sascha. > > ---snip--- > > >>> diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c new file >>> mode 100644 index 0000000..5830368 >>> --- /dev/null >>> +++ b/drivers/net/smc911x.c >>> > > ---snip--- > > >>> + >>> +#ifdef CONFIG_DRIVER_SMC911X >>> + >>> >>> >> This should be moved to the Makefile. >> > > Agreed > > > ---snip--- > > >>> >>> >> Register and bitfield definitions should be in a header file. >> > > Not these file specific ones. > Ben - where else would they be applicable? > > Well, I can't come up with a better answer than 'precedent', so I guess it's OK to keep the #defines in the C code. >> More generally, only register addresses and bitfields should >> be defined. >> > > Using macros to encapsulate both address and > >> function is bad form, IMHO. >> > > Agreed > > >> I haven't even gotten into the functionality, because I think >> there's a lot of work to be done just in coding style >> > > Ben - perhaps you could help by pointing out some more examples > > By coding style I mean the nasty macros, not stuff like brackets and whitespace. If the code was more readable and addressed the bus width differences between chips, this would probably go in quickly. I'll have another look to see if I can be more helpful rather than curmudgeonly. regards, Ben