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