[PATCH] Respond:Add SUNIX-Multi-I/O card device driver
From: Morris Ku
Date: Wed Mar 13 2019 - 09:31:40 EST
Hi Enrico,
Thanks for review, my replies are inline:
Signed-off-by: Morris Ku <saumah@xxxxxxxxx>
---
+On 08.03.19 13:35, Morris Ku wrote:
+> This patch add header file, Kconfig and Makefile.
+>
+> Signed-off-by: Morris Ku <saumah@xxxxxxxxx>
+> ---
+> 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.<info@xxxxxxxxxxxx>"
+> +#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