drivers/staging: refactor dgnc tty registration.
PATCH 1/1 drivers/staging: refactor dgnc tty registration.
-remove duplicate tty allocation code for serial and printer drivers. -fix sparse warning: too long initializer-string for array of char.
--- drivers/staging/dgnc/dgnc_driver.h | 13 ---- drivers/staging/dgnc/dgnc_tty.c | 137 ++++++++++++++----------------------- 2 files changed, 50 insertions(+), 100 deletions(-)
drivers/staging/dgnc/dgnc_driver.h +0 -13
@@ -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
drivers/staging/dgnc/dgnc_tty.c +50 -87
165 lines not on this page · raw message
Signature · 1 line
1.9.1
drivers/staging/dgnc/dgnc_tty.c static void dgnc_tty_set_termios(struct tty_struct *tty, · line 116
Quoted 5 lines · Haim Daniel
drivers/staging/dgnc/dgnc_tty.c +1
-0@@ -129,6 +113,41 @@ static void dgnc_tty_set_termios(struct tty_struct *tty,/* TTY Initialization/Cleanup Functions */ +static struct tty_driver *__dgnc_tty_register(char *serial_name, uint maxports,
No need for double underscores.
drivers/staging/dgnc/dgnc_tty.c static void dgnc_tty_set_termios(struct tty_struct *tty, · line 125
Quoted 9 lines
drivers/staging/dgnc/dgnc_tty.c +9
-0+ int major, int minor) +{ + int rc; +struct tty_driver *drv =tty_alloc_driver (maxports, +TTY_DRIVER_REAL_RAW | +TTY_DRIVER_DYNAMIC_DEV | +TTY_DRIVER_HARDWARE_BREAK ); +if (IS_ERR (drv)) +return drv;
Blank line between declaration and code. Do it like this:
struct tty_driver *drv; int rc; drv = tty_alloc_driver(maxports, TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV | TTY_DRIVER_HARDWARE_BREAK); if (IS_ERR(drv)) return drv;
Also create a matching dgnc_tty_unregister function that does:
static void dgnc_tty_unregister(struct tty_driver *drv)
{
tty_unregister_driver(drv);
put_tty_driver(drv);
}regards,
dan carpenter
PATCH v2 1/1 drivers/staging: refactor dgnc tty registration.
-remove duplicate tty allocation code for serial and printer drivers. -fix sparse warning: too long initializer-string for array of char.
--- drivers/staging/dgnc/dgnc_driver.h | 13 ---- drivers/staging/dgnc/dgnc_tty.c | 150 +++++++++++++++---------------------- 2 files changed, 59 insertions(+), 104 deletions(-)
drivers/staging/dgnc/dgnc_driver.h +0 -13
@@ -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
drivers/staging/dgnc/dgnc_tty.c +59 -91
182 lines not on this page · raw message
Signature · 1 line
1.9.1
Quoted 1 line · Haim Daniel
+static struct
tty_driver*_dgnc_tty_register(char*serial_name, uint maxports,
Ahh... I see why you are adding underscores. Please, just get a better name instead of taking an existing name and adding underscores.
regards,
dan carpenter
PATCH v3 1/1 drivers/staging: refactor dgnc tty registration.
-remove duplicate tty allocation code for serial and printer drivers. -fix sparse warning: too long initializer-string for array of char.
--- drivers/staging/dgnc/dgnc_driver.h | 13 ---- drivers/staging/dgnc/dgnc_tty.c | 150 +++++++++++++++---------------------- 2 files changed, 59 insertions(+), 104 deletions(-)
drivers/staging/dgnc/dgnc_driver.h +0 -13
@@ -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
drivers/staging/dgnc/dgnc_tty.c +59 -91
182 lines not on this page · raw message
Signature · 1 line
1.9.1
Review
Quoted 2 lines · Haim Daniel
(quoted diff) +0
-2-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.
Quoted 42 lines
Signed-off-by Haim Daniel <[email protected]>--- drivers/staging/dgnc/dgnc_driver.h | 13 ---- drivers/staging/dgnc/dgnc_tty.c | 150 +++++++++++++++---------------------- 2 files changed, 59 insertions(+), 104 deletions(-)drivers/staging/dgnc/dgnc_driver.h +0
-13@@ -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 drivers/staging/dgnc/dgnc_tty.c +0
-3@@ -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.
drivers/staging/dgnc/dgnc_tty.c line 66
Quoted 10 lines
drivers/staging/dgnc/dgnc_tty.c +0
-10-* -* 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?
Quoted 23 lines
drivers/staging/dgnc/dgnc_tty.c +13
-3- .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; +
drivers/staging/dgnc/dgnc_tty.c static void dgnc_tty_set_termios(struct tty_struct *tty, · line 138
Quoted 10 lines
drivers/staging/dgnc/dgnc_tty.c +10
-0+ 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.
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.
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.
I still feel a bit bad about my review that I didn't spot bug Sparse saw...
regards,
dan carpenter
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
Quoted 46 lines · Dan Carpenter
Review
Quoted 2 lines · Haim Daniel
(quoted diff) +0
-2-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.
Quoted 39 lines
Signed-off-by Haim Daniel <[email protected]>--- drivers/staging/dgnc/dgnc_driver.h | 13 ---- drivers/staging/dgnc/dgnc_tty.c | 150 +++++++++++++++---------------------- 2 files changed, 59 insertions(+), 104 deletions(-)drivers/staging/dgnc/dgnc_driver.h +0
-13@@ -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 drivers/staging/dgnc/dgnc_tty.c +0
-0@@ -51,22 +51,6 @@ .digi_term = "ansi" /* default terminal type */ };
Quoted 15 lines
drivers/staging/dgnc/dgnc_tty.c line 66
Quoted 13 lines
drivers/staging/dgnc/dgnc_tty.c +0
-13-/* -* 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.
Quoted 29 lines
Quoted 29 lines
drivers/staging/dgnc/dgnc_tty.c +18
-3- .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 ;
Quoted 8 lines
drivers/staging/dgnc/dgnc_tty.c static void dgnc_tty_set_termios(struct tty_struct *tty, · line 138
Quoted 5 lines
drivers/staging/dgnc/dgnc_tty.c +5
-0+ 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_ispeedandc_ospeedis almost certainly correct, but they're not mentioned in the changelog.
Ack, will do.
Quoted 3 lines
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 :)
Quoted 3 lines
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.
Quoted 7 lines
I still feel a bit bad about my review that I didn't spot bug Sparse saw...
regards,
dan carpenter
Quoted 10 lines · Haim Daniel
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
Quoted 6 lines
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
I downloaded the latest Sparse and don't see that warning. But GCC has a warning like that so I think this is really a GCC warning. It doesn't trigger for me, and with my config it's in fact, not a bug. The buffer is 19 characters and INIT_C_CC is 17 chars plus 1 NUL terminator.
All these things are generic termios defines so I'm not even sure how you're triggering the warning.
Anyway, your patch is harmless enough.
regards,
dan carpenter
PATCH v4 1/1 drivers/staging: refactor dgnc tty registration.
In reply to Dan Carpenter-remove duplicate tty allocation code for serial and printer drivers. -add missing tty c_ispeed and c_ospeed initialization to 9600. -fix sparse warning: too long initializer-string for array of char.
This patch was only unit tested due to lack of the actual hardware.
--- drivers/staging/dgnc/dgnc_driver.h | 13 ---- drivers/staging/dgnc/dgnc_tty.c | 150 +++++++++++++++---------------------- 2 files changed, 59 insertions(+), 104 deletions(-)
drivers/staging/dgnc/dgnc_driver.h +0 -13
@@ -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
drivers/staging/dgnc/dgnc_tty.c +59 -91
182 lines not on this page · raw message
Signature · 1 line
1.9.1
Review
Quoted 3 lines · Haim Daniel
(quoted diff) +0
-3-remove duplicate tty allocation code for serial and printer drivers. -add missing tty c_ispeed and c_ospeed initialization to 9600. -fix sparse warning: too long initializer-string for array of char.
This is a GCC warning and not a Sparse warning. I still have no idea how it's getting triggered. But otherwise I have reviewed the patch and it's a nice clean up.
regards,
dan carpenter