Hi Enrico, Thanks for review, my replies are inline: Signed-off-by: Morris Ku <[email protected]> --- +On 08.03.19 13:35, Morris Ku wrote: +> This patch add header file, Kconfig and Makefile. +> +> Signed-off-by: Morris Ku <[email protected]> +> --- +> char/snx/Kconfig | 15 + +> char/snx/Makefile | 9 + +> char/snx/driver_extd.h | 170 ++++++ +> char/snx/snx_common.h | 1157 ++++++++++++++++++++++++++++++++++++++++ +> char/snx/snx_lp.h | 126 +++++ +> char/snx/snx_ppdev.h | 43 ++ + +why isn't that in ./drivers/tty/serial/sunix/ ? + driver support SUNIX Character Devices, serial ports and parallel ports,so we suggest that in /drivers/char. + +<snip> + +> diff --git a/char/snx/Kconfig b/char/snx/Kconfig +> new file mode 100644 +> index 00000000..86f38352 +> --- /dev/null +> +++ b/char/snx/Kconfig +> @@ -0,0 +1,15 @@ +> +# SPDX-License-Identifier: GPL-2.0 +> +# +> +# Character device configuration +> +# +> + +> +config SNX + +please use full name: SUNIX +
Ok, i'll fix in next verion. + +<snip> + +> diff --git a/char/snx/driver_extd.h b/char/snx/driver_extd.h +> new file mode 100644 +> index 00000000..27f69570 +> --- /dev/null +> +++ b/char/snx/driver_extd.h +> @@ -0,0 +1,170 @@ +> +/* SPDX-License-Identifier: GPL-2.0 */ +> + +> +#ifndef _SNXHW_DRVR_EXTR_H_ +> +#define _SNXHW_DRVR_EXTR_H_ +> + +> +#ifndef SNX_IOCTL +> +#define SNX_IOCTL 0x900 +> +#endif +> + +> +#define SNX_COMM_GET_BOARD_CNT (SNX_IOCTL + 100) +> +#define SNX_COMM_GET_BOARD_INFO (SNX_IOCTL + 101) +> + +> +#define SNX_GPIO_GET (SNX_IOCTL + 200) +> +#define SNX_GPIO_SET (SNX_IOCTL + 201) +> +#define SNX_GPIO_READ (SNX_IOCTL + 202) +> +#define SNX_GPIO_WRITE (SNX_IOCTL + 203) +> +#define SNX_GPIO_SET_DEFAULT (SNX_IOCTL + 204) +> +#define SNX_GPIO_WRITE_DEFAULT (SNX_IOCTL + 205) +> +#define SNX_GPIO_GET_INPUT_INVERT (SNX_IOCTL + 206) +> +#define SNX_GPIO_SET_INPUT_INVERT (SNX_IOCTL + 207) +> + +> +#define SNX_UART_GET_TYPE (SNX_IOCTL + 300) +> +#define SNX_UART_SET_TYPE (SNX_IOCTL + 301) +> +#define SNX_UART_GET_ACS (SNX_IOCTL + 302) +> +#define SNX_UART_SET_ACS (SNX_IOCTL + 303) + +why exactly do you introduce driver-specific ioctls ? + ioctl functions support SUNIX specific serial,parellel and GPIO ,need to use specific ioctol function to get related inforomation. + +what is "ACS" + RS485 ACS Enable,Enable SUNIX UART Controller waiting transmit data when receive data is going. + +> +#define SNX_GPIO_IN 0 +> +#define SNX_GPIO_OUT 1 +> +#define SNX_GPIO_LOW 0 +> +#define SNX_GPIO_HI 1 +> +#define SNX_GPIO_INPUT_INVERT_D 0 +> +#define SNX_GPIO_INPUT_INVERT_E 1 + +GPIO stuff belongs into the gpio subsystem. see drivers/gpio/ + SUNIX Multi-I/O card combaine serial ports,parallel ports and GPIO, therefore, the define support SUNIX specific gpio interface. + +<snip> + +> diff --git a/char/snx/snx_common.h b/char/snx/snx_common.h +> new file mode 100644 +> index 00000000..16dd8f02 +> --- /dev/null +> +++ b/char/snx/snx_common.h +> @@ -0,0 +1,1157 @@ +> +???// SPDX-License-Identifier: GPL-2.0 +> + +> +#ifdef MODVERSIONS +> +#ifndef MODULE +> +#define MODULE +> +#endif +> +#endif + +dont need that. please drop it. + Ok, i will drop it. + +<snip> + +> +#ifndef KERNEL_VERSION +> +#define KERNEL_VERSION(ver, rel, seq) ((ver << 16) | (rel << 8) | seq) +> +#endif + +same here. + Ok, i will drop it. + +> +#include <linux/errno.h> +> +#include <linux/signal.h> +> +#include <linux/tty.h> +> +#include <linux/tty_flip.h> +> +#include <linux/serial.h> +> +#include <linux/serial_reg.h> +> +#include <linux/ioport.h> +> +#include <linux/mm.h> +> +#include <linux/kernel.h> +> +#include <linux/init.h> +> +#include <linux/slab.h> +> +#include <linux/wait.h> +> +#include <linux/tty_driver.h> +> +#include <linux/pci.h> +> +#include <linux/circ_buf.h> +> +#include <asm/uaccess.h> +> +#include <asm/io.h> +> +#include <asm/irq.h> +> +#include <asm/segment.h> +> +#include <asm/serial.h> +> +#include <linux/interrupt.h> +> +#include <linux/module.h> +> +#include <linux/workqueue.h> +> +#include <linux/moduleparam.h> +> +#include <linux/console.h> +> +#include <linux/sysrq.h> +> +#include <linux/delay.h> +> +#include <linux/device.h> +> +#include <linux/kref.h> +> +#include <linux/parport.h> +> +#include <linux/ctype.h> +> +#include <linux/poll.h> +> +#include <linux/sched.h> +> +#include <linux/serial_8250.h> +> +#include <linux/cdev.h> +> +#include <linux/sched/signal.h> + +are these really all needed within the header ? + i will fix it. + +> +extern int snx_board_count; + +Why that extern field ? + i will fix it. + +> +#define SNX_DRIVER_VERSION "V2.0.4.5" +> +#define SNX_DRIVER_AUTHOR "SUNIX Co., Ltd.<[email protected]>" +> +#define SNX_DRIVER_DESC "SUNIX Multi-I/O Board Driver Module" + +Does it need to be in here ? (see MODULE_*() macros) + Ok, i will moved to appropriate file. + +<snip> + +> +typedef struct _PORT { +> + char type; +> + +> + int bar1; +> + unsigned char offset1; +> + unsigned char length1; +> + +> + int bar2; +> + unsigned char offset2; +> + unsigned char length2; +> + +> + unsigned int intmask; +> + unsigned int chip_flag; +> + +> +} PORT; + +please no uppercase type names, and use proper prefix. + ok , i will fix it + +> +#if defined(__i386__) && (defined(CONFIG_M386) || \ +> +defined(CONFIG_M486)) +> +#define SNX_SERIAL_INLINE +> +#endif +> + +> +#ifdef SNX_SERIAL_INLINE +> +#define _INLINE_ inline +> +#else +> +#define _INLINE_ +> +#endif + +what is that for ?! + i will drop it. + +> +struct snx_ser_driver { +> + struct module *owner; +> + const char *driver_name; +> + const char *dev_name; +> + int major; +> + int minor; +> + int nr; +> + struct snx_ser_state *state; +> + struct tty_driver *tty_driver; +> +}; + +oh, not good. the struct tty_driver should be contained in +snx_ser_driver (not a pointer to it). or the other way round: +give the tty driver a pointer to a driver-private struct. + +and this data looks as it should be assigned to individual +devices, not to driver globally. + ok , i will fix it + +> +#include <linux/tty_flip.h> + +put all includes together at the top + ok , i will fix it + + +> +static inline void snx_ser_insert_char +> +( +> + struct snx_ser_port *port, +> + unsigned int status, +> + unsigned int overrun, +> + unsigned int ch, +> + unsigned int flag +> +) +> +{ +> + struct snx_ser_info *info = port->info; +> + struct tty_struct *tty = info->tty; +> + struct snx_ser_state *state = NULL; +> + struct tty_port *tport = NULL; +> + +> + state = tty->driver_data; +> + +> + tport = &state->tport; +> + +> + if ((status & port->ignore_status_mask & ~overrun) == 0) { +> + +> + if (tty_insert_flip_char(tport, ch, flag) == 0) +> + ++port->icount.buf_overrun; +> + } +> + +> + if (status & ~port->ignore_status_mask & overrun) { +> + +> + if (tty_insert_flip_char(tport, 0, TTY_OVERRUN) == 0) +> + ++port->icount.buf_overrun; +> + } +> +} + +too big for an inline function. + ok , i will fix it + +> +#define SNX_CONFIG_PARPORT_1284 +> +#define SNX_CONFIG_PARPORT_PC_FIFO +> + +> +#ifdef SNX_CONFIG_PARPORT_1284 +> +#undef SNX_CONFIG_PARPORT_1284 +> +#endif +> + +> +#ifdef SNX_CONFIG_PARPORT_PC_FIFO +> +#undef SNX_CONFIG_PARPORT_PC_FIFO +> +#endif + +parport, serial port, gpio should be separate drivers. + driver support SUNIX Multi-I/O card(ex: 1-port serial + 1-port Parallel), therefore, i prefer to keep it in one driver. + +> +struct snx_parport_ops { + +Why does the driver introduce its own callback vectors ? + function definition in multiple .c files. + +> + +> +struct sunix_par_port { +> + struct snx_parport *port; +> + +> + unsigned char ctr; +> + unsigned char ctr_writable; +> + int ecr; +> + int fifo_depth; +> + int pword; +> + int read_intr_threshold; +> + int write_intr_threshold; +> + +> + char *dma_buf; + +why not void * ? + i am not sure what you mean ? + +> + dma_addr_t dma_handle; +> + struct list_head list; +> + +> + unsigned long base; +> + unsigned long base_hi; +> + int irq; +> + int portnum; +> + unsigned int snx_type; +> + +> + int board_enum; +> + unsign +ed int bus_number; +> + unsigned int dev_number; +> + +> + PCI_BOARD pb_info; +> + +> + unsigned char chip_flag; +> + unsigned int port_flag; +> +}; +> + +> +// snx_devtable.c +> +extern PCI_BOARD snx_pci_board_conf[]; +<snip> + +why all these extern functions ? + function definition in multiple .c files. + +<snip> + +> +extern char snx_ser_ic_table[SNX_SER_PORT_MAX_UART][10]; +> +extern char snx_par_ic_table[SNX_PAR_PORT_MAX_UART][10]; +> +extern char snx_port_remap[2][10]; + +please try not to use global fields. these things look they belong into +the corresponding device private data structs. + ok , i will fix it + +> +#define sunix_parport_write_data(p, x) sunix_parport_pc_write_data(p, x) +> +#define sunix_parport_read_data(p) sunix_parport_pc_read_data(p) +> +#define sunix_parport_write_control(p, x) \ +> +sunix_parport_pc_write_control(p, x) +> +#define sunix_parport_read_control(p) sunix_parport_pc_read_control(p) +> +#define sunix_parport_frob_control(p, m, v) \ +> +sunix_parport_pc_frob_control(p, m, v) +> +#define sunix_parport_read_status(p) \ +> +sunix_parport_pc_read_status(p) +> +#define sunix_parport_enable_irq(p) sunix_parport_pc_enable_irq(p) +> +#define sunix_parport_disable_irq(p) sunix_parport_pc_disable_irq(p) +> +#define sunix_parport_data_forward(p) sunix_parport_pc_data_forward(p) +> +#define sunix_parport_data_reverse(p) sunix_parport_pc_data_reverse(p) + +does that really need to be in a header file ? + I suggest that keep it in a header file. + +<snip> + +> +static inline unsigned char sunix_parport_pc_frob_control( +> +struct snx_parport *p, unsigned char mask, unsigned char val) +> +{ +> + const unsigned char wm = (PARPORT_CONTROL_STROBE | +> + PARPORT_CONTROL_AUTOFD | +> + PARPORT_CONTROL_INIT | +> + PARPORT_CONTROL_SELECT); +> + +> + if (mask & 0x20) { +> + if (val & 0x20) +> + sunix_parport_pc_data_reverse(p); +> + else +> + sunix_parport_pc_data_forward(p); +> + +> + } +> + +> + mask &= wm; +> + val &= wm; +> + +> + return __sunix_parport_pc_frob_control(p, mask, val); +> +} + +do these functions really *need* to be in the header and static inline ? +(IOW: are they really needed from multiple .c files ?) + I suggest that keep it in a header file. + +> diff --git a/char/snx/snx_lp.h b/char/snx/snx_lp.h +> new file mode 100644 +> index 00000000..8c48deea +> --- /dev/null +> +++ b/char/snx/snx_lp.h + +<snip> + +> +#define __KERNEL__ 1 +> + +> +#ifdef __KERNEL__ + +doesn't belong here. drop that. + ok , i will fix it + +> + +> +#include <linux/mutex.h> + +put this at the top. if it's really needed here. + ok , i will fix it + +> diff --git a/char/snx/snx_ppdev.h b/char/snx/snx_ppdev.h +> new file mode 100644 +> index 00000000..635f99e1 +> --- /dev/null +> +++ b/char/snx/snx_ppdev.h +> @@ -0,0 +1,43 @@ +> +/* SPDX-License-Identifier: GPL-2.0 */ +> +#include "snx_common.h" +> + +> +#define SNX_PP_IOCTL 'p' +> + +> +struct snx_ppdev_frob_struct { +> + unsigned char mask; +> + unsigned char val; +> +}; +> + +> + +> +#define SNX_PPSETMODE _IOW(SNX_PP_IOCTL, 0x80, int) +> +#define SNX_PPRSTATUS _IOR(SNX_PP_IOCTL, 0x81, unsigned char) +> +#define SNX_PPRCONTROL _IOR(SNX_PP_IOCTL, 0x83, unsigned char) +> +#define SNX_PPWCONTROL _IOW(SNX_PP_IOCTL, 0x84, unsigned char) +> +#define SNX_PPFCONTROL _IOW(SNX_PP_IOCTL, 0x8e,\ +> +struct snx_ppdev_frob_struct) +> +#define SNX_PPRDATA _IOR(SNX_PP_IOCTL, 0x85, unsigned char) +> +#define SNX_PPWDATA _IOW(SNX_PP_IOCTL, 0x86, unsigned char) +> +#define SNX_PPCLAIM _IO(SNX_PP_IOCTL, 0x8b) +> +#define SNX_PPRELEASE _IO(SNX_PP_IOCTL, 0x8c) +> +#define SNX_PPYIELD _IO(SNX_PP_IOCTL, 0x8d) +> +#define SNX_PPEXCL _IO(SNX_PP_IOCTL, 0x8f) +> +#define SNX_PPDATADIR _IOW(SNX_PP_IOCTL, 0x90, int) +> +#define SNX_PPNEGOT _IOW(SNX_PP_IOCTL, 0x91, int) +> +#define SNX_PPWCTLONIRQ _IOW(SNX_PP_IOCTL, 0x92, unsigned char) +> +#define SNX_PPCLRIRQ _IOR(SNX_PP_IOCTL, 0x93, int) +> +#define SNX_PPSETPHASE _IOW(SNX_PP_IOCTL, 0x94, int) +> +#define SNX_PPGETTIME _IOR(SNX_PP_IOCTL, 0x95, struct timeval) +> +#define SNX_PPSETTIME _IOW(SNX_PP_IOCTL, 0x96, struct timeval) +> +#define SNX_PPGETMODES _IOR(SNX_PP_IOCTL, 0x97, unsigned int) +> +#define SNX_PPGETMODE _IOR(SNX_PP_IOCTL, 0x98, int) +> +#define SNX_PPGETPHASE _IOR(SNX_PP_IOCTL, 0x99, int) +> +#define SNX_PPGETFLAGS _IOR(SNX_PP_IOCTL, 0x9a, int) +> +#define SNX_PPSETFLAGS _IOW(SNX_PP_IOCTL, 0x9b, int) +> + +> +#define SNX_PP_FASTWRITE (1<<2) +> +#define SNX_PP_FASTREAD (1<<3) +> +#define SNX_PP_W91284PIC (1<<4) +> + +> +#define SNX_PP_FLAGMASK (SNX_PP_FASTWRITE | SNX_PP_FASTREAD \ + +what exactly are these needed for ? + ok , i will fix it -- 2.17.1
