From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965219AbdEONtX (ORCPT ); Mon, 15 May 2017 09:49:23 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:36005 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933582AbdEONtV (ORCPT ); Mon, 15 May 2017 09:49:21 -0400 Subject: Re: [PATCH v3] drivers/staging: refactor dgnc tty registration. To: Dan Carpenter References: <20170515115841.33uyk5xx26a773g7@mwanda> <1494851450-29629-1-git-send-email-haimdaniel@gmail.com> <20170515125252.2u6fqqfupoixbsui@mwanda> Cc: lidza.louina@gmail.com, markh@compro.net, gregkh@linuxfoundation.org, driverdev-devel@linuxdriverproject.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org From: Haim Daniel Message-ID: <9efb4f29-5097-91bd-8aba-7fd247f65a5c@gmail.com> Date: Mon, 15 May 2017 16:49:16 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170515125252.2u6fqqfupoixbsui@mwanda> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org me@haim-toshiba1 ~ $ dpkg -l sparse Desired=Unknown/Install/Remove/Purge/Hold | Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad) ||/ Name Version Architecture Description +++-=================-=============-=============-======================================== ii sparse 0.4.5~rc1-1 amd64 semantic parser of source files me@haim-toshiba1 linux (next-20170512) $ make clean M=drivers/staging /dgnc/; make C=1 M=drivers/staging/dgnc/ drivers/staging/dgnc/dgnc_tty.c:66:25: warning: too long initializer-string for array of char On 05/15/2017 03:52 PM, Dan Carpenter wrote: > On Mon, May 15, 2017 at 03:30:50PM +0300, Haim Daniel wrote: >> -remove duplicate tty allocation code for serial and printer drivers. >> -fix sparse warning: too long initializer-string for array of char. > > I think my version of Sparse is too old. I don't get a warning. Please > cut and paste the exact warning. > >> >> Signed-off-by: Haim Daniel >> --- >> drivers/staging/dgnc/dgnc_driver.h | 13 ---- >> drivers/staging/dgnc/dgnc_tty.c | 150 +++++++++++++++---------------------- >> 2 files changed, 59 insertions(+), 104 deletions(-) >> >> diff --git a/drivers/staging/dgnc/dgnc_driver.h b/drivers/staging/dgnc/dgnc_driver.h >> index 980410f..764d6fe 100644 >> --- a/drivers/staging/dgnc/dgnc_driver.h >> +++ b/drivers/staging/dgnc/dgnc_driver.h >> @@ -52,19 +52,6 @@ >> >> #define dgnc_jiffies_from_ms(a) (((a) * HZ) / 1000) >> >> -/* >> - * Define a local default termios struct. All ports will be created >> - * with this termios initially. This is the same structure that is defined >> - * as the default in tty_io.c with the same settings overridden as in serial.c >> - * >> - * In short, this should match the internal serial ports' defaults. >> - */ >> -#define DEFAULT_IFLAGS (ICRNL | IXON) >> -#define DEFAULT_OFLAGS (OPOST | ONLCR) >> -#define DEFAULT_CFLAGS (B9600 | CS8 | CREAD | HUPCL | CLOCAL) >> -#define DEFAULT_LFLAGS (ISIG | ICANON | ECHO | ECHOE | ECHOK | \ >> - ECHOCTL | ECHOKE | IEXTEN) >> - >> #ifndef _POSIX_VDISABLE >> #define _POSIX_VDISABLE '\0' >> #endif >> diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c >> index 9e98781..d3736da 100644 >> --- a/drivers/staging/dgnc/dgnc_tty.c >> +++ b/drivers/staging/dgnc/dgnc_tty.c >> @@ -51,22 +51,6 @@ >> .digi_term = "ansi" /* default terminal type */ >> }; >> >> -/* >> - * Define a local default termios struct. All ports will be created >> - * with this termios initially. >> - * >> - * This defines a raw port at 9600 baud, 8 data bits, no parity, >> - * 1 stop bit. >> - */ >> -static const struct ktermios default_termios = { >> - .c_iflag = (DEFAULT_IFLAGS), >> - .c_oflag = (DEFAULT_OFLAGS), >> - .c_cflag = (DEFAULT_CFLAGS), >> - .c_lflag = (DEFAULT_LFLAGS), >> - .c_cc = INIT_C_CC, > > We don't need INIT_C_CC? No, the idea was reusing the tty_std_termios, and setting only the relevant params. > >> - .c_line = 0, >> -}; >> - >> static int dgnc_tty_open(struct tty_struct *tty, struct file *file); >> static void dgnc_tty_close(struct tty_struct *tty, struct file *file); >> static int dgnc_block_til_ready(struct tty_struct *tty, struct file *file, >> @@ -129,6 +113,49 @@ static void dgnc_tty_set_termios(struct tty_struct *tty, >> >> /* TTY Initialization/Cleanup Functions */ >> >> +static struct tty_driver *dgnc_tty_create(char *serial_name, uint maxports, >> + int major, int minor) >> +{ >> + int rc; >> + struct tty_driver *drv; >> + >> + drv = tty_alloc_driver(maxports, >> + TTY_DRIVER_REAL_RAW | >> + TTY_DRIVER_DYNAMIC_DEV | >> + TTY_DRIVER_HARDWARE_BREAK); >> + if (IS_ERR(drv)) >> + return drv; >> + >> + drv->name = serial_name; >> + drv->name_base = 0; >> + drv->major = major; >> + drv->minor_start = minor; >> + drv->type = TTY_DRIVER_TYPE_SERIAL; >> + drv->subtype = SERIAL_TYPE_NORMAL; >> + drv->init_termios = tty_std_termios; >> + drv->init_termios.c_cflag = (B9600 | CS8 | CREAD | HUPCL | CLOCAL); >> + drv->init_termios.c_ispeed = 9600; >> + drv->init_termios.c_ospeed = 9600; > > Setting c_ispeed and c_ospeed is almost certainly correct, but they're > not mentioned in the changelog. Ack, will do. > Btw, the overwhelming vast majority of staging drivers are sent without > testing. But it looks like maybe you are testing yours? Please mention > that in the changelog because it makes us feel warm inside. I only unit tested a fraction of the code, will mention it in the changelog, for the sake of that fuzzy feeling :) > Otherwise it looks fine to me. Sorry for making you redo it so many > times instead of reviewing thouroughly all at once. I shouldn't have > been so lazy. np. > I still feel a bit bad about my review that I didn't spot bug Sparse > saw... > > regards, > dan carpenter > >