On Fri, Aug 17, 2012 at 2:29 PM, Benoît Thébaudeau benoit.thebaudeau@advansee.com wrote:
Dear Matt Sealey,
- Use iomux-mx51.h include to simplify board configuration.
- Remove LED toggle function as it had no real users.
- Red LED is now on for pre-reloc, Blue LED for "in U-Boot"
- Function renames for readability.
- Some board identification string changes
- Reduce CPU core voltage to 1.1V (per TO3 spec)
- Implicitly remove support for TO2 boards
These are many unrelated changes that could be split.
Could be, but they're not going to be, the reasoning behind it being that it increases work for us to test each individual patch works in any particular order it may be committed to any tree, and increases work for patch maintainers whereby there are too many inter-dependent changes, extra fuzz, rebasing etc. which is just totally needless. In essence this is a cleanup patch intended to get rid of some things that made absolutely no sense but have persisted upstream for the longest time. It also ports to iomux-mx51.h as previously sent since that helps a lot on cleanup.
We tested every single part of this as a unit internally over 10 weeks, what's being sent to the list is the result. What isn't in the patch (or more accurately, what got removed) was code that would make more of a mess for very little functionality, code that was repeated or overcomplicated for no good reason (led setup for example) or was over-abstracted.
What went in to the process was the old code: functionality was tested and verified against expectations. What you get from the new code: almost exactly the same behavior (actually it works a little better as some redundant things were removed).
The sign-off is not just me hitting "-s" on my git command line, it's my explicit assurance that this is highly tested code that I am willing to maintain and answer questions on. Marex has been asking me to submit it for a long, long time and I refused because we were still going through QA; that got finished, to a level I am happy with at least (still a few weird things happening but they existed with the old code too). However, QA got done, and the result is clear, and splitting it up means going through QA again just to respin patches, so I'm going to refuse on that basis alone. If we just consider it a cleanup patch for the board and a reasonable example of the benefits of the new iomux model code, does that make it easier to accept?
Signed-off-by: Matt Sealey matt@genesi-usa.com
[snip]
MX51_PAD_CSPI1_SS1__GPIO4_25,MX51_PAD_GPIO1_6__GPIO1_6,+};
Why don't you merge all these pad settings to a single board pad array, with a single call to imx_iomux_v3_setup_multiple_pads()?
That has been done before (it was the way we did it in our ancient Linux kernels, it was the way Freescale did it in their ancient Linux kernels). We feel a huge board-specific array is hard to maintain and ugly to read, and the few differences would end up in tiny, separate arrays anyway. The U-Boot code was written that way in the first place, it made a lot of sense to keep the status quo for now. My preferred solution would be to split each part into a seperate source file and call them from the board file (as efikamx-usb.c is done now, but for MMC, ATA too) where such code would clean up the main board file significantly, but I think that would be too big a change for a lot of people. We can perform a more egregious cleanup later.