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@denx.de Cc: Marek Vasut marek.vasut@gmail.com Cc: Tom Rini trini@ti.com Cc: Xu, Hong Hong.Xu@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