[U-Boot] [PATCH 5/7 v6] serial: add S3C64XX serial driver

[Guennadi Liakhovetski]

On Mon, 11 Aug 2008, Wolfgang Denk wrote:

> Dear Guennadi Liakhovetski,
> 
> In message <Pine.LNX.4.64.0808111042400.30591 at 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 at denx.de