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

["Andreas Bießmann" <andreas.devel@googlemail.com>]

Dear Marek Vasut,

On 17.09.2012 12:00, Marek Vasut wrote:
> 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.

Ok.

<snip>

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

Ok.

<snip>

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

Ah, ok. Sounds good.

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

Understood, I'm fine with it. I will run a test on avr32/some at91 board
these days and send my ACK then.

Best regards

Andreas Bie?mann