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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
