* [PATCH] Respond:Add SUNIX-Multi-I/O card device driver
@ 2019-03-13 13:31 Morris Ku
2019-03-13 17:36 ` Enrico Weigelt, metux IT consult
0 siblings, 1 reply; 2+ messages in thread
From: Morris Ku @ 2019-03-13 13:31 UTC (permalink / raw)
To: gregkh; +Cc: morris_ku, linux-kernel, arnd, Morris Ku
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 14286 bytes --]
Hi Enrico,
Thanks for review, my replies are inline:
Signed-off-by: Morris Ku <saumah@gmail.com>
---
+On 08.03.19 13:35, Morris Ku wrote:
+> This patch add header file, Kconfig and Makefile.
+>
+> Signed-off-by: Morris Ku <saumah@gmail.com>
+> ---
+> 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@sunix.com.tw>"
+> +#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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] Respond:Add SUNIX-Multi-I/O card device driver
2019-03-13 13:31 [PATCH] Respond:Add SUNIX-Multi-I/O card device driver Morris Ku
@ 2019-03-13 17:36 ` Enrico Weigelt, metux IT consult
0 siblings, 0 replies; 2+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-03-13 17:36 UTC (permalink / raw)
To: Morris Ku, gregkh; +Cc: morris_ku, linux-kernel, arnd
On 13.03.19 14:31, Morris Ku wrote:
Hi,
> +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.
Well, this seems to be a composite device. so it should be actually
different drivers (initialized by one compound driver) - all of the
subdevices I'm seeing here have their own subsystems, none of them
being a plain chardev. Therefore it probably should go to drivers/mfd
(multi function devices). If you split out the subdevice drivers
(which I'd recommend), these should go to the corresponding subsystem
directories (eg. drivers/tty/serial/ for the serial subdevice).
> +please use full name: SUNIX
> +
>
> Ok, i'll fix in next verion.
Oh, by the way: SUNIX is the company name ? So, there's probably some
device/product name. I'd put that into the config name, too.
Eg. if the device is called "FANCYIO", then the config symbol would
be SUNIX_FANCYIO.
> +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.
Which information, exactly, that aren't supported by the corresponding
subsystems ?
To make it clear: the individual functions of this card should go into
the corresponding subsystem. So, you'd have a serial driver, a parallel
driver, a gpio driver - all living in the corresponding subsystems.
NOT: one driver (more precisely: one chardev) to rule them all.
Note: in Linux we wanna use generic APIs where we can. So, if somebody
wants to use a GPIOs, he goes by the GPIO subsystem - no matter which
hardware it actually is - no hw specific devices/ioctl.
> +what is "ACS"
> +
> RS485 ACS Enable,Enable SUNIX UART Controller waiting transmit
> data when receive data is going.
See struct uart_port and corresponding helpers (drivers/tty/serial),
it already has infrastructure for that. It also does all the buffering
stuff for you.
> SUNIX Multi-I/O card combaine serial ports,parallel ports and GPIO,
> therefore, the define support SUNIX specific gpio interface.
See above: don't introduce your own specific interfaces - use the
generic ones. Seriously, that's the way it's going on Linux.
> +> + char *dma_buf;
> +
> +why not void * ?
> +
> i am not sure what you mean ?
Is it really correct that the dma_buf has to be char* ?
(and even w/o signed/unsigned attribute).
For opaque memory buffers, we usually use void*.
> +> +// snx_devtable.c
> +> +extern PCI_BOARD snx_pci_board_conf[];
> +<snip>
> +
> +why all these extern functions ?
> +
> function definition in multiple .c files.
it's not a function, but an array of structs.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-03-13 17:36 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-13 13:31 [PATCH] Respond:Add SUNIX-Multi-I/O card device driver Morris Ku
2019-03-13 17:36 ` Enrico Weigelt, metux IT consult
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).