Re: [PATCH v3] drivers/staging: refactor dgnc tty registration.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <haimdaniel@xxxxxxxxx>
---
 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




_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux