From: Wolfgang Denk Date: Sun, 23 Aug 2009 20:53:51 +0200 Subject: [U-Boot] [RFC PATCH 1/3] serial: join stdio_dev and serial_device In-Reply-To: <1251034019-10787-1-git-send-email-plagnioj@jcrosoft.com> References: <20090823125957.GC11950@game.jcrosoft.org> <1251034019-10787-1-git-send-email-plagnioj@jcrosoft.com> Message-ID: <20090823185351.2BA35833DBD2@gemini.denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Dear Jean-Christophe PLAGNIOL-VILLARD, In message <1251034019-10787-1-git-send-email-plagnioj@jcrosoft.com> you wrote: > we use for the serail multi api the struct stdio_dev also which will reduce > and simplify the code and prepare the join of all serial APIs. > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD > --- > board/esd/pmc440/pmc440.c | 2 +- > board/lwmon/lwmon.c | 2 +- > board/omap3/zoom2/zoom2_serial.h | 18 +++--- > board/trizepsiv/conxs.c | 8 ++-- > common/serial.c | 89 +++++++++++++++---------------- > common/stdio.c | 3 - > cpu/mpc5xxx/serial.c | 36 ++++++------ > cpu/mpc8xx/serial.c | 36 ++++++------ > cpu/ppc4xx/4xx_uart.c | 38 +++++++------- > drivers/serial/serial.c | 24 ++++---- > drivers/serial/serial_pxa.c | 54 +++++++++--------- > drivers/serial/serial_s3c24x0.c | 22 ++++---- > include/serial.h | 109 ++++++++++++++++--------------------- > include/stdio_dev.h | 9 +++- > 14 files changed, 218 insertions(+), 232 deletions(-) > rewrite include/serial.h (65%) > > diff --git a/board/esd/pmc440/pmc440.c b/board/esd/pmc440/pmc440.c > index 9ffb08e..de01e93 100644 > --- a/board/esd/pmc440/pmc440.c > +++ b/board/esd/pmc440/pmc440.c > @@ -53,7 +53,7 @@ int is_monarch(void); > int bootstrap_eeprom_read(unsigned dev_addr, unsigned offset, > uchar *buffer, unsigned cnt); > > -struct serial_device *default_serial_console(void) > +struct stdio_dev *default_serial_console(void) I don't think the renaming of "serial" into "stdio" is a good idea. "stdio" is a more specific term, which can only be applied to the standard input/output channes (sdin, stdout and stderr); in other words, this is restricted to the serial ports used as console interface. However, "serial" is a more general term - in addition to the stdio devices this may be used for example for additional serial ports controlling other things. ... > -int serial_register (struct serial_device *dev) > +int serial_register (struct stdio_dev *dev) > { > - dev->init += gd->reloc_off; > + dev->flags = DEV_FLAGS_OUTPUT | DEV_FLAGS_INPUT | DEV_FLAGS_SERIAL; Please see comment below about DEV_FLAGS_SERIAL. > -void serial_stdio_init (void) > +static struct stdio_dev* serial_get_by_name(char* name) > { > - struct stdio_dev dev; > - struct serial_device *s = serial_devices; > + struct stdio_dev *dev = stdio_get_by_name(name); Here it becomes clear that your renaming of serial into stdio is not consistent; to me it makes no sense. Why is it serial_get_by_name() [note: "serial"], when it actually returns a "struct stdio_dev*"? You call stdio_get_by_name() internally? That does not amke sense. How can you get a device from a wider class (serial devices), when you consider only a smaller selection (stio devices)? Hmmm... What is a "stdio device" by the way? When we use a USB keyboard and assign it as stdin, it is a stdio device, right? when we assing a LCD outpout as stdout and stderr, it is a stdio device, right? > --- a/include/stdio_dev.h > +++ b/include/stdio_dev.h > @@ -32,14 +32,19 @@ > > #define DEV_FLAGS_INPUT 0x00000001 /* Device can be used as input console */ > #define DEV_FLAGS_OUTPUT 0x00000002 /* Device can be used as output console */ > +#define DEV_FLAGS_SERIAL 0x00000003 /* Device is a serial device */ No. This makes no sense at all. DEV_FLAGS_* are bit names. DEV_FLAGS_SERIAL migfht be 4, but in no case it makes sense to use 3 here, which is "DEV_FLAGS_INPUT | DEV_FLAGS_OUTPUT". > /* Device information */ > struct stdio_dev { > int flags; /* Device flags: input/output/system */ > int ext; /* Supported extensions */ > - char name[16]; /* Device name */ > + char name[NAMESIZE]; /* Device name */ > + char ctlr[CTLRSIZE]; Comment? What's ctlr[CTLRSIZE] supposed to mean? NAK. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de A witty saying proves nothing, but saying something pointless gets people's attention.