From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Mon, 17 Sep 2012 12:00:46 +0200
Subject: [U-Boot] [PATCH 41/71] serial: arm: Implement
	CONFIG_SERIAL_MULTI into atmel serial driver
In-Reply-To: <5056C869.8010801@googlemail.com>
References: <1347837696-3192-1-git-send-email-marex@denx.de>
	<1347837696-3192-42-git-send-email-marex@denx.de>
	<5056C869.8010801@googlemail.com>
Message-ID: <201209171200.47104.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear 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.
[...]

> > -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.

We can't rework the whole stdio/serial subsystem right away. All such calls 
(serial_setbrg, serial_putc etc) will be augmented by one more parameter to push 
such information through at runtime. This will be done in subsequent patch, 
stage 1 in only a preparation stage.

> >  	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?

Indeed, but only in stage 2 or somewhere later ... I have that in mind, but the 
serial subsystem needs a bit of a patching for that.

> > -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')?

This is the name of the driver, not the name of the instance of the driver. I'd 
like to add .id field later on.

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

No, this isn't instance. This are driver ops combined with it's name. I can not 
split it yet.

> 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?

Yes, but not now, not yet. I'm trying to keep this changes incremental as much 
as possible.

> 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.

They have multiple drivers so far and a default_serial_console call. That is 
indeed stupid, but fixing this is not part of this patchset, but a subsequent 
one. This one is only a preparation, trying not to break anything and unify the 
drivers under the serial_multi api, so the further stages can easily continue 
reworking it.

> Best regards
> 
> Andreas Bie?mann