Peter, On Thu, Mar 17, 2011 at 7:32 AM, Peter Tyser <pty...@xes-inc.com> wrote: > Hi Tom, > >> >> > >> >> >> + /* Is PLL-X already running? */ >> >> >> + reg = readl(&clkrst->crc_pllx_base); >> >> >> + if (reg & PLL_ENABLE) >> >> >> + return; >> >> >> + >> >> >> + /* Do PLLX init if it isn't running, but BootROM sets it, so TBD >> >> >> */ >> >> >> +} >> >> > >> >> > The above function looks incorrect. >> >> What looks incorrect? It checks to see if the PLL is already >> >> running/enabled, and returns if it is. >> >> Tegra2 mask ROM (BootROM) currently always sets PLLX, so this will >> >> always return, but the comment is there for future chips that may not >> >> set up PLLX. >> > >> > It looks like a function that does a read of a value it doesn't care >> > about, does an unnecessary comparison, and then returns either way, >> > without doing anything:) If/when you want to do future stuff with the >> > PLL-X, code should be added then - as is it doesn't do anything and is >> > confusing. The general policy of U-Boot is not to add dead code. >> OK, so not really incorrect, just unnecessary. Agreed - again a >> vestigial leftover from our in-house code. I'll remove it. > > Unnecessary/misleading/dead code is inherently not correct:) > > <snip> > >> >> >> +#include <asm/types.h> >> >> >> + >> >> >> +#ifndef FALSE >> >> >> +#define FALSE 0 >> >> >> +#endif >> >> >> +#ifndef TRUE >> >> >> +#define TRUE 1 >> >> >> +#endif >> >> > >> >> > These shouldn't be added here. They should be somewhere common, or >> >> > shouldn't be used (my preference). >> >> I would think they'd be in the ARM tree somewhere, but I couldn't find >> >> them so I added 'em here. >> >> My preference is to use TRUE/FALSE - it carries more context than 1/0 to >> >> me. >> > >> > If you prefer to use TRUE/FALSE, they should be moved into a common >> > location so everywhere uses the same, once-defined definition. Their >> > definitions are already littered over a few files, which would ideally >> > be cleaned up. Perhaps moving all definitions into include/common.h, or >> > somewhere similar would work. Others may have opinions about TRUE/FALSE >> > vs 1/0 - it seems like TRUE/FALSE aren't generally used. >> I don't want to pollute all builds by adding to include/common.h. >> I'll try to find a more central header in my own tree. > > My point is that there are already 32 definitions of TRUE/FALSE - adding > a 33rd doesn't seem like a good thing to do. I view a 33rd identical > definition as polluting the code more than 1 common definition that most > people won't generally use. > > Its not my decision, but I assume the powers that be would recommend one > of: > - Not using TRUE/FALSE since using non-zero values and 0 are widely > accepted as TRUE/FALSE. I think using TRUE/FALSE makes the code harder > to understand and more open to bugs. Eg for other code that interacts > with your code, or someone reviewing your code, they either have to > either assume you defined TRUE as 1, FALSE as 0, or import your > definitions. Anyway, I view their use as another layer of unnecessary > abstraction with very little benefit. I've removed both the defines and the use of TRUE/FALSE in ap20.* - there were only a few instances.
> > - Consolidating the definition of TRUE/FALSE. > > Wolfgang, do you have a preference about how TRUE/FALSE are generally > used/defined? > > Best, > Peter Thanks, Tom > > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot