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

Marek Vasut
2012-09-17T10:00:46Z

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