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 <ma...@denx.de>
> Cc: Marek Vasut <marek.va...@gmail.com>
> Cc: Tom Rini <tr...@ti.com>
> Cc: Xu, Hong <hong...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
