On Mon, 11 Aug 2008, Wolfgang Denk wrote:
Dear Guennadi Liakhovetski,
In message Pine.LNX.4.64.0808111042400.30591@axis700.grange you wrote:
- u32 reg;
- u32 pclk_ratio = get_PCLK() / gd->baudrate;
- int i;
IMHO it's still obscur
- /* PCLK / (16 * baudrate) - 1 */
- reg = pclk_ratio / 16 - 1;
I don't see any baudrate in this calculation. From the comment I would expect to see
reg = pclk_ratio / (16 * gd->baudrate) - 1;
or similar. For the reader the comment is misleading as it is difficult to figure out that "PCLK" and "pclk_ratio" are not the same thing.
Ok, pclk_ratio is, probably, not the best name for this.
- /* i = pclk_ratio % 16 */
- i = pclk_ratio - (reg + 1) * 16;
I don't see any % operation in this calculation. From the comment I would expect to see
i = pclk_ratio % 16;
or similar. Either the comment or the code or both are wrong.
I think, they are both correct by the way pclk_ration is calculated.
Sorry, I don't understand how this can be made clearer yet. If you have any specific ideas, please let me know now, before v7 appears, because I would really like to make it final.
Well, maybe the code and the comments should be in sync. Actually, if the comment just says the same as the code, it is redundant and should be omitted. If the comment tries to describe what the code implements in a different way that should be made clear, too.
Probably it doesn't make much sense to try to factor out common expressions when it makes thew code more difficult to read - the compiler is pretty clever about such things, too.
Ok, would this be better
u32 reg; u32 pclk = get_PCLK(); u32 baudrate = gd->baudrate; int i;
reg = pclk / baudrate / 16 - 1; i = (pclk / baudrate) % 16;
This way the compiler does optimize it to only one division, and the operations say what they do, and there doesn't seem to be any need for comment left. Still, the code produced by this is 1 asm instruction longer than my original code:-)
Thanks Guennadi --- Guennadi Liakhovetski, Ph.D.
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de