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

[Guennadi Liakhovetski]

On Mon, 4 Aug 2008, Jean-Christophe PLAGNIOL-VILLARD wrote:

> On 14:45 Mon 04 Aug     , Guennadi Liakhovetski wrote:
> > Ported from u-boot-1.1.6 driver by Samsung.
> > 
> > Signed-off-by: Guennadi Liakhovetski <lg at denx.de>
> > ---
> > 
> > Also added my copyright to the driver.
> > 
> >  drivers/serial/Makefile  |    1 +
> >  drivers/serial/s3c64xx.c |  189 ++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 190 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/serial/s3c64xx.c
> > 
> > diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> > index c9e797e..2a11ae5 100644
> > --- a/drivers/serial/Makefile
> > +++ b/drivers/serial/Makefile
> > @@ -30,6 +30,7 @@ COBJS-y += mcfuart.o
> >  COBJS-y += ns9750_serial.o
> >  COBJS-y += ns16550.o
> >  COBJS-y += s3c4510b_uart.o
> > +COBJS-$(CONFIG_S3C64XX) += s3c64xx.o
> >  COBJS-y += serial.o
> >  COBJS-y += serial_max3100.o
> >  COBJS-y += serial_pl010.o
> I've send a patch that break it.
> 
> Could you rebase it on the current Wolfgang tree HEAD?

Ok, will have a look.

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

> > +void serial_setbrg(void)
> > +{
> > +	DECLARE_GLOBAL_DATA_PTR;
> > +	s3c64xx_uart *const uart = s3c64xx_get_base_uart(UART_NR);
> > +	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.

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

> > +
> > +	uart->UBRDIV = reg;
> > +	uart->UDIVSLOT = udivslot[i];
> base on the cose
> 	uart->UDIVSLOT = udivslot[0];

See above.

> > +static int hwflow;		/* turned off by default */
> why not?
> static int hwflow = 0;		/* turned off by default */

Because static are always initialised to 0, and, in fact, checkpatch.pl 
produces warnings on these, and rightfully so.

> > +static int be_quiet;
> why not?
> static int be_quiet = 0;

See above.

Thanks for your comments
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