[U-Boot-Users] [PATCH 5/7 v3] serial: add s3c64xx serial driver

[Wolfgang Denk]

In message <Pine.LNX.4.64.0808050004180.10557 at axis700.grange> you wrote:
> 
> > > +/* See table in 31.6.11 */
> > > +static const int udivslot[] = {
> > > +	0,
> > > +	0x0080,
> > > +	0x0808,
> > > +	0x0888,
> > > +	0x2222,
> > > +	0x4924,
> > > +	0x4a52,
> > > +	0x54aa,
> > > +	0x5555,
> > > +	0xd555,
> > > +	0xd5d5,
> > > +	0xddd5,
> > > +	0xdddd,
> > > +	0xdfdd,
> > > +	0xdfdf,
> > > +	0xffdf,
> > > +};
> > Can we have something more readable?
> 
> No. This are "recommended values" as mentioned in the comment to the table 
> referenced above.

Bout perhaps you could add some documentation what  all  these  magic
numbers  mean? Of course we can all download the documetnation, study
it for hours and finally uinderstand this, too  -  but  it  would  be
nicer if you could save us this effort.

> > > +	u32 reg, pclk_ratio = get_PCLK() / gd->baudrate;
> > why not
> > 	u32 reg;
> > 	u32 pclk_ratio = get_PCLK() / gd->baudrate;
> 
> There is more than one way to do it.

Indeed. But the one that is better readable is preferred.

> > > +	/* PCLK / (16 * baudrate) - 1 */
> > > +	reg = pclk_ratio / 16 - 1;
> > > +	i = pclk_ratio - (reg + 1) * 16;
> > =>
> > 	i = pclk_ratio - (pclk_ratio / 16 - 1 + 1) * 16;
> > =>
> > 	i = pclk_ratio - (pclk_ratio / 16 ) * 16;
> > =>
> > 	i = pclk_ratio - pclk_ratio;
> > =>
> > 	i = 0;
> 
> Please, think again. This is integer arithmetics, not analysis.

So perhaps you want to elucidate your code in a comment?

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
grep me no patterns and I'll tell you no lines.