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

[Wolfgang Denk]

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.

> > > +	/* 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.

> 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.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Though a program be but three lines long,
someday it will have to be maintained."
- The Tao of Programming