[U-Boot] [PATCH 41/71] serial: arm: Implement CONFIG_SERIAL_MULTI into atmel serial driver

[Andreas Bießmann]

Dear Marek Vasut,

I have to admit that when reading this patch I got attention of your
UDM-serial.txt for the first time. However when reading this patch some
questions come to my mind.

On 17.09.12 01:21, Marek Vasut wrote:
> Implement support for CONFIG_SERIAL_MULTI into atmel serial driver.
> This driver was so far only usable directly, but this patch also adds
> support for the multi method. This allows using more than one serial
> driver alongside the atmel driver. Also, add a weak implementation
> of default_serial_console() returning this driver.
> 
> Signed-off-by: Marek Vasut <marex at denx.de>
> Cc: Marek Vasut <marek.vasut at gmail.com>
> Cc: Tom Rini <trini at ti.com>
> Cc: Xu, Hong <Hong.Xu at atmel.com>
> ---
>  common/serial.c              |    2 ++
>  drivers/serial/atmel_usart.c |   67 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 63 insertions(+), 6 deletions(-)
> 
> diff --git a/common/serial.c b/common/serial.c
> index e6566da..b880303 100644
> --- a/common/serial.c
> +++ b/common/serial.c
> @@ -71,6 +71,7 @@ serial_initfunc(sconsole_serial_initialize);
>  serial_initfunc(p3mx_serial_initialize);
>  serial_initfunc(altera_jtag_serial_initialize);
>  serial_initfunc(altera_serial_initialize);
> +serial_initfunc(atmel_serial_initialize);
>  
>  void serial_register(struct serial_device *dev)
>  {
> @@ -120,6 +121,7 @@ void serial_initialize(void)
>  	p3mx_serial_initialize();
>  	altera_jtag_serial_initialize();
>  	altera_serial_initialize();
> +	atmel_serial_initialize();
>  
>  	serial_assign(default_serial_console()->name);
>  }
> diff --git a/drivers/serial/atmel_usart.c b/drivers/serial/atmel_usart.c
> index 943ef70..d49d5d4 100644
> --- a/drivers/serial/atmel_usart.c
> +++ b/drivers/serial/atmel_usart.c
> @@ -20,6 +20,8 @@
>   */
>  #include <common.h>
>  #include <watchdog.h>
> +#include <serial.h>
> +#include <linux/compiler.h>
>  
>  #include <asm/io.h>
>  #include <asm/arch/clk.h>
> @@ -29,7 +31,7 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> -void serial_setbrg(void)
> +static void atmel_serial_setbrg(void)
>  {
>  	atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;

shouldn't this USART_BASE be carried by the driver struct in some way? I
wonder how one should implement multiple interfaces later on with this
atmel_serial_xx(void) interface.

>  	unsigned long divisor;
> @@ -45,7 +47,7 @@ void serial_setbrg(void)
>  	writel(USART3_BF(CD, divisor), &usart->brgr);
>  }
>  
> -int serial_init(void)
> +static int atmel_serial_init(void)
>  {
>  	atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
>  
> @@ -73,7 +75,7 @@ int serial_init(void)
>  	return 0;
>  }
>  
> -void serial_putc(char c)
> +static void atmel_serial_putc(char c)
>  {
>  	atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
>  
> @@ -84,13 +86,13 @@ void serial_putc(char c)
>  	writel(c, &usart->thr);
>  }
>  
> -void serial_puts(const char *s)
> +static void atmel_serial_puts(const char *s)
>  {
>  	while (*s)
>  		serial_putc(*s++);
>  }

I have seen this one in a lot of drivers ... shouldn't we build a
generic one?

>  
> -int serial_getc(void)
> +static int atmel_serial_getc(void)
>  {
>  	atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
>  
> @@ -99,8 +101,61 @@ int serial_getc(void)
>  	return readl(&usart->rhr);
>  }
>  
> -int serial_tstc(void)
> +static int atmel_serial_tstc(void)
>  {
>  	atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
>  	return (readl(&usart->csr) & USART3_BIT(RXRDY)) != 0;
>  }
> +
> +#ifdef CONFIG_SERIAL_MULTI
> +static struct serial_device atmel_serial_drv = {
> +	.name	= "atmel_serial",

even though here is just one instance shouldn't the name reflect the
multiplicity of this driver (e.g. 'atmel_serial0')?

> +	.start	= atmel_serial_init,
> +	.stop	= NULL,
> +	.setbrg	= atmel_serial_setbrg,
> +	.putc	= atmel_serial_putc,
> +	.puts	= atmel_serial_puts,
> +	.getc	= atmel_serial_getc,
> +	.tstc	= atmel_serial_tstc,

As I understand this struct we need a start/stop/setbgr/... for each
instance we build.
Shouldn't we carry some void* private in this struct instead (I have
none seen in '[PATCH 01/71] serial: Coding style cleanup of struct
serial_device') to be able to reuse the interface with multiple
instances of the same driver class?
I think this is my main objection to this structure. I wonder how
existing SERIAL_MULTI implementations handle the need of private driver
information bound to an instance.

Best regards

Andreas Bießmann