Dear Keith,
in message
OF0EE85153.67A4AEC8-ON072571E6.005FB093-072571E6.006006D2@mck.us.ray.com you wrote:
I have submitted four patches against U-Boot to Wolfgang Denk directly
due
to list attachment size limits.
Can you please update your patches using a current code base? I get a lot of rejects when applying your patches.
OK. I was hoping I could get away with not having to do that. Sorry!
Also, please perform the following cleanup / changes:
- Make sure to adapt the Makefiles to the new build environment extensions (BUILD_DIR and depend).
OK
- Please create just a single 'drivers/xilinx' directory instead of 5; use subdirectories where really necessary
OK. I should have done this the first time.
- Clean up the Coding style (trailing white space in 65 files, C++ comments, indentation not by tabs, trailing empty lines, etc.)
I'll run all the sources through GNU indent.
Please add correct author information; "Author: Xilinx, Inc." is not acceptable.
Please clean up your license headers. The GPL is pretty clear that there must not be any additional restrictions on the code. Combining this with statements like "YOU ARE RESPONSIBLE FOR OBTAINING ANY THIRD PARTY RIGHTS YOU MAY REQUIRE" is IMHO not acceptable. Either this is GPL, and I have all rights, or it isn't. Also, "Use in such applications is expressly prohibited." is a restriction that is IMO incompatible with the GPL.
Perhaps the current git head does not have these statements in the Xilinx code in ./board/xilinx/xilinx_enet, for example. I'll check tonight. My source base does have them, though, but my code is a little old.
- Please clean up the function headers and comments. Something like
this:
126
/*****************************************************************************/
127 /** 128 * 129 * Sets up a callback function to be invoked when an assert
occurs. If there
130 * was already a callback installed, then it is replaced. 131 * 132 * @param Routine is the callback to be invoked when an assert
is taken
133 * 134 * @return 135 * 136 * None. 137 * 138 * @note 139 * 140 * This function has no effect if NDEBUG is set 141 * 142
******************************************************************************/
143 void 144 XAssertSetCallback(XAssertCallback Routine) 145 { 146 XAssertCallbackRoutine = Routine; 147 }(i. e. 17 lines of mostly empty comment versus 1 line of code) is not acceptable. Please be descriptive, but terse.
This code is *terrybly* bloated - so much, that it's actually unreadable.
I tend to agree, but this is the Xilinx provided driver code. I can take not credit for it's quality :)
BTW, Some of this code is already in U-Boot (./board/xilinx/common ./board/xilinx_emac ./board/xilinx_iic). My plan was to put it into a common location after which anyone else using the code in ./boards/xilinx could switch over to the code in ./drivers
Is there any chance to clean this up such that only the *necessary* stuff gets included? You don;t want to tell me that all this is needed or even used?
The idea was to use the driver source code as-is to avoid having to re-invent the wheel. I would expect that as more Xilinx FPGA based boards are added, the percentage use of the Xilinx driver code would increase. I actually do use a lot of the functionality provided by the drivers because one of my primary uses of U-Boot is to assist in hardware design verification and debug. So I end up having to write lots of diagnostic code to examine hardware when, for example, I am trying to get the Xilinx SystemAce to work properly. Also, the systems I work on do not tend to be resource constrained. A luxury, to be sure, but it means I can tolerate larger executables.
Please resubmit after cleanup. Thanks.
OK.
Best regards,
Wolfgang Denk
-- Software Engineering: Embedded and Realtime Systems, Embedded Linux Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "The bad reputation UNIX has gotten is totally undeserved, laid on by people who don't understand, who have not gotten in there and tried anything." -- Jim Joyce, owner of Jim Joyce's UNIX Bookstore