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