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/ ? <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 <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 ? what is "ACS" > +#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/ <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. <snip> > +#ifndef KERNEL_VERSION > +#define KERNEL_VERSION(ver, rel, seq) ((ver << 16) | (rel << 8) | seq) > +#endif same here. > +#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 ? > +extern int snx_board_count; Why that extern field ? > +#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) <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. > +#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 ?! > +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. > +#include <linux/tty_flip.h> put all includes together at the top > +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. > +#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. > +struct snx_parport_ops { Why does the driver introduce its own callback vectors ? > + > +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 * ? > + 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; > + unsigned 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 ? <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. > +#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 ? <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 ?) > 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. > + > +#include <linux/mutex.h> put this at the top. if it's really needed here. > 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 ? --mtx -- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering [email protected] -- +49-151-27565287
