* [PATCH] Respond:add support for SUNIX Multi-I/O board
@ 2019-03-15 10:06 Morris Ku
2019-03-20 18:24 ` Enrico Weigelt, metux IT consult
0 siblings, 1 reply; 6+ messages in thread
From: Morris Ku @ 2019-03-15 10:06 UTC (permalink / raw)
To: gregkh; +Cc: morris_ku, linux-kernel, arnd, Morris Ku
Hi Enrico,
Thanks for review, my replies are inline:
Signed-off-by: Morris Ku <saumah@gmail.com>
---
+From 5b1c4c8f7d91661a27c88b980c42b768e4cb7606 Mon Sep 17 00:00:00 2001
+From: Morris Ku <saumah@gmail.com>
+Date: Tue, 26 Feb 2019 17:11:48 +0800
+Subject: [PATCH 6/6] add support for SUNIX Multi-I/O board
+
+
+<snip>
+
+> diff --git a/char/snx/snx_main.c b/char/snx/snx_main.c
+
+if it's a serial card, shouldn't it go to drivers/tty/serial/snx/ ?
+
i will move to drivers/mfd (multi function devices)
+
+
+<snip>
+
+> +char
+[SNX_SER_PORT_MAX_UART][10] = {
+> + {"UNKNOWN"},
+> + {"SUN1889"},
+> + {"SUN1699"},
+> + {"SUNMATX"},
+> + {"SUN1999"}
+> +};
+> +
+> +char snx_par_ic_table[SNX_PAR_PORT_MAX_UART][10] = {
+> + {"UNKNOWN"},
+> + {"SUN1888"},
+> + {"SUN1689"},
+> + {"SUNMATX"},
+> + {"SUN1999"}
+> +};
+> +
+> +char snx_port_remap[2][10] = {
+> + {"NON-REMAP"},
+> + {"REMAP"}
+> +};
+
+can't these be const static ?
+
i will fix it.
+
+can we have a bit more consistent formatting (eg. all those comments
+w/ same indention) ?
+
i prefer to keep it in current format.
+
+> + SUN1999_BOARD_5008A,
+> +
+
+<snip>
+
+> +static struct pci_device_id sunix_pci_board_id[] = {
+> +// golden-serial
+
+can't this be const ?
+
i will fix it.
+
+(maybe even __initconst)
+I'd really prefer separate port types - that go into separate
+subsystems, in separate modules (those can share code in a common
+module, if desired)
+
+Oh, is there really a hard restriction on the max number of boards ?
+
+driver support maximum 4 boards can be installed incombination
+(up to 32 serial port and 2 parallel port)
+
+And do we really need a global list of them ? (instead of just having
+all per-board / per-port data in a per-board / per-port driver instance)
+
+
i prefer to keep current format.
+
+
+> +static int snx_ser_port_total_cnt;
+> +static int snx_par_port_total_cnt;
+> +
+> +int snx_board_count;
+> +
+> +static struct snx_ser_driver sunix_ser_reg = {
+> + .dev_name = "ttySNX",
+> + .major = 0,
+> + .minor = 0,
+> + .nr = (SNX_SER_TOTAL_MAX + 1),
+> +};
+
+can't this be const ?
+
i prefer keep it in current format.
+
+> +static irqreturn_t sunix_interrupt(int irq, void *dev_id)
+> +{
+> + struct sunix_ser_port *sp = NULL;
+> + struct sunix_par_port *pp = NULL;
+> + struct sunix_board *sb = NULL;
+> + int i;
+
+> + int status = 0;
+> +
+> + int handled = IRQ_NONE;
+> +
+> + for (i = 0; i < SNX_BOARDS_MAX; i++) {
+> +
+> + if (dev_id == &(sunix_board_table[i])) {
+> + sb = dev_id;
+> + break;
+> + }
+> + }
+
+maybe put this index into the board data, so the loop on the global
+array isn't needed ?
+
i prefer keep it in current format.
+
+<snip>
+
+> + if ((sb->ser_port > 0) && (sb->ser_isr != NULL)) {
+> + sp = &sunix_ser_table[sb->ser_port_index];
+
+maybe put this pointer struct sunix_board so the lookup in the
+global array isn't necessary ?
+
i prefer keep it in current format.
+
+<snip>
+
+> + if ((sb->par_port > 0) && (sb->par_isr != NULL)) {
+> + pp = &sunix_par_table[sb->par_port_index];
+> +
+> + if (!pp)
+> + status = 1;
+> +
+> + status = sb->par_isr(sb, pp);
+> + }
+> +
+> + if (status != 0)
+> + return handled;
+> +
+> + handled = IRQ_HANDLED;
+> + return handled;
+> +}
+
+btw: is there only one irq per board or one per port ?
+
only one irq per board.
+
+> +static int snx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+> +{
+> + return 0;
+
+shouldn't probe check whether the device is actually there and then do
+the initialization ?
+
i will fix it.
+
+<snip>
+
+> +static int snx_resume_port_termios(struct snx_ser_info *info)
+> +{
+> + struct snx_ser_state *state = NULL;
+> + struct tty_struct *tty = info->tty;
+> +
+> + state = tty->driver_data;
+> + snx_set_port_termios(state);
+> +
+> + return 0;
+> +}
+> +
+> +
+
+do we need multiple newlines here ?
+
i will fix it.
+
+<snip>
+
+> +static int snx_resume_one(struct pci_dev *pdev)
+> +{
+> + struct sunix_board *sb = pci_get_drvdata(pdev);
+> + struct sunix_ser_port *sp = NULL;
+> + int j;
+> +
+> + if (sb == NULL)
+> + return 0;
+> +
+> + for (j = 0; j < sb->ser_port; j++) {
+> + sp = &sunix_ser_table[j];
+
+can't the pointers to the serial ports be in struct sunix_board instead
+of an global array ?
+
+if it already is a global, why not statically initialized ?
+Why not doing this in snx_pci_probe() on per-board basis, and let device
+registration be done by pci subsystem ?
+
i prefer keep it in current format.
+
+> + // search sun1999 muti I/O board
+> + pdev = NULL;
+> + tablecnt = 0;
+> + sub_device_id = 0;
+> + status = 0;
+> + device_part_number = 0;
+> + bar3_base_add = 0;
+> + bar3_Byte5 = 0;
+> + bar3_Byte6 = 0;
+> + bar3_Byte7 = 0;
+> + oem_id = 0;
+> + uart_type = 0;
+> + gpio_type = 0;
+> + gpio_card_type = 0;
+> + gpio_ch_cnt = 0;
+
+completely different boards handled by one big driver ?
+why not splitting it up into multiple drivers ?
+
SUNIX has many differing cards,serail,parallel and
multi-i/o(serial + parellel), therefore, we combaine to one driver.
+
+<snip>
+
+> + if ((gpio_ch_cnt == 0x00) && (gpio_card_type == 0x01))
+> + gpio_ch_cnt = 6;
+> + else if ((gpio_ch_cnt == 0x00) && (gpio_card_type == 0x02))
+> + gpio_ch_cnt = 8;
+> + else if (gpio_ch_cnt == 0x01)
+> + gpio_ch_cnt = 16;
+> + else if (gpio_ch_cnt == 0x02)
+> + gpio_ch_cnt = 32;
+> +
+
+gpio devices have their own subsystem -> therefore: write a separate
+gpio driver for that.
+
ioctl functions support for SUNIX specific serial,parellel and cash drawer
interface.
+
+<snip>
+why that limitation ? why does this have to be a global array at all ?
+
driver support maximum 4 boards can be installed incombination
(up to 32 serial port and 2 parallel port)
+
+> +static int sunix_assign_resource(void)
+> +{
+> + struct sunix_board *sb = NULL;
+> + struct sunix_ser_port *sp = NULL;
+> + struct sunix_par_port *pp = NULL;
+> +
+> + int status = 0;
+> + int i;
+> + int j;
+> + int k;
+> + int ser_n;
+> + int ser_port_index = 0;
+> +
+> + memset(sunix_ser_table, 0, (SNX_SER_TOTAL_MAX + 1) *
+> + sizeof(struct sunix_ser_port));
+> +
+> + memset(sunix_par_table, 0, (SNX_PAR_TOTAL_MAX) *
+> + sizeof(struct sunix_par_port));
+
+why this w/ global variables, instead of on per-device basis on device-
+private data ?
+
i prefer keep it in current format.
+
+<snip>
+
+> +static int sunix_ser_port_table_init(void)
+> +{
+
+same here.
+
+<snip>
+
+> +static int sunix_par_port_table_init(void)
+> +{
+
+same here
+
+> +int sunix_register_irq(void)
+> +{
+
+same here.
+
+<snip>
+why not doing those initializations on per-device basis, in the
+corresponding probe() function ?
+use devm_*() functions, which automatically releases resource when a
+device is deleted, so explicit release isn't needed anymore.
+
i will fix it.
+
+> +static void __exit snx_exit(void)
+> +{
+> + if (snx_par_port_total_cnt > 0) {
+> + sunix_par_lp_exit();
+> + sunix_par_ppdev_exit();
+> + sunix_par_parport_exit();
+> + }
+
+Let the cleanup be done by the individual driver's release functions,
+and down forget to set .owner correctly - that way, the kernel won't
+even allow unloading the module, as long as it's in use. then, you'd
+probably don't need much more than just removing unregistering the
+drivers here.
+
i prefer keep it in current format.
+
+> diff --git a/char/snx/snx_serial.c b/char/snx/snx_serial.c
+> new file mode 100644
+> index 00000000..fdac4c90
+> --- /dev/null
+> +++ b/char/snx/snx_serial.c
+> @@ -0,0 +1,4263 @@
+> +// SPDX-License-Identifier: GPL-2.0
+> +#include "snx_common.h"
+> +#include "driver_extd.h"
+> +
+> +#define SNX_ioctl_DBG 0
+
+what exactly is that ioctl for ?
+or more to the point: why are you introducing a driver specific iotctl ?
+
i will drop it.
+
+> +#define EEPROM_ACCESS_DELAY_COUNT 100000
+> +
+> +static DEFINE_SEMAPHORE(ser_port_sem);
+> +
+> +#define SNX_HIGH_BITS_OFFSET ((sizeof(long)-sizeof(int))*8)
+> +#define sunix_ser_users(state) \
+> +((state)->count + ((state)->info ? (state)->info->blocked_open : 0))
+> +
+> +static struct tty_port snx_service_port;
+
+why has this to be global, instead of in per-device private data ?
+
I am not sure what you mean ?
+
+<snip>
+
+> +static _INLINE_ void snx_ser_handle_cts_change(
+> + struct snx_ser_port *, unsigned int);
+
+Where's this strange "_INLINE_" coming from ?
+
i will fix it.
+
+<snip>
+
+> +//extern void snx_ser_change_speed(
+> + //struct snx_ser_state *state, struct SNXTERMIOS *old_termios);
+
+please no dead/commented-out code.
+
i will drop it.
+
+> +//extern int sunix_ser_interrupt(
+> + //struct sunix_board *, struct sunix_ser_port *first_sp);
+
+same here
+
i will drop it.
+
+> +//extern int sunix_ser_register_ports(struct snx_ser_driver *drv);
+> +//extern void sunix_ser_unregister_ports(struct snx_ser_driver *drv);
+> +//extern int sunix_ser_register_driver(struct snx_ser_driver *drv);
+> +//extern void sunix_ser_unregister_driver(struct snx_ser_driver *drv);
+
+same here
+
i will drop it.
+
+> +static unsigned char READ_INTERRUPT_VECTOR_BYTE(struct sunix_ser_port *sp)
+
+i'd recommend no caps in function names.
+
i will fix it.
+
+> + if (var == 0x04) {
+> + vet4 = inb(local_vector + 0xb0);
+> + vet4 <<= 12;
+> + }
+> +
+> + data = (vet1 | vet2 | vet3 | vet4);
+> +
+> + return data;
+> + }
+> + return 0;
+> +}
+> +
+> +
+> +
+
+remove excess newlines
+
i will fix it.
+
+<snip>
+
+> +static int EEPROMWriteData(int targetConfigAddress, int address, int data)
+
+why do you use int instead of void* for addresses ?
+oh, and shouldn't it be __iomem ?
+
+targetConfigAddress for PCI (bar)base address register.
+
+<snip>
+
+> + Error = inb(targetConfigAddress + 0x08) & 0x04;
+
+<snip>
+
+> +static void snx_ser_start(struct tty_struct *tty)
+> +{
+> + int line = SNX_SER_DEVNUM(tty);
+> +
+> + if (line >= SNX_SER_TOTAL_MAX)
+> + return;
+> +
+> + //spin_lock_irqsave(&port->lock, flags);
+> + __snx_ser_start(tty);
+> + //spin_unlock_irqrestore(&port->lock, flags);
+
+why are the spinlock calls commented out ?
+
i wiil fix it.
+
+<nsip>
+
+> + tasklet_kill(&info->tlet);
+
+what exactly is the tasklet needed for ?
+
i will fix it.
+
+> + if (!capable(CAP_SYS_ADMIN)) {
+> + retval = -EPERM;
+
+why this explicit check for CAP_SYS_ADMIN ?
+
check users with admin rights
+
+> + if (change_irq ||
+
+indention mismatch.
+
i will fix it.
+
+> + change_port ||
+
+why is userland allowed to change irq, port, etc, if that's probed
+from pci anyways ?
+
+
Since changing the 'type' of the port changes its resource
allocations, we should treat type changes the same as
IO port changes.
+
+>
+
+ (new_serial.baud_base != port->uartclk / 16) ||
+> + (close_delay != state->close_delay) ||
+> + (closing_wait != state->closing_wait) ||
+> + (new_serial.xmit_fifo_size != port->fifosize) ||
+> + (((new_flags ^ old_flags) & ~SNX_UPF_USR_MASK) != 0)) {
+> + goto exit;
+> + }
+> +
+> + port->flags = ((port->flags & ~SNX_UPF_USR_MASK) |
+> + (new_flags & SNX_UPF_USR_MASK));
+> + port->custom_divisor = new_serial.custom_divisor;
+> + goto check_and_exit;
+> + }
+
+<snip>
+
+> +static int snx_ser_ioctl(struct tty_struct *tty,
+> +unsigned int cmd, unsigned long arg)
+> +{
+<snip>
+> + case TIOCSSERIAL:
+> + {
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + state->port->setserial_flag = SNX_SER_BAUD_SETSERIAL;
+> + ret = snx_ser_set_info(state,
+> + (struct serial_struct *)arg);
+> + }
+> + break;
+> + }
+> +
+> +
+> + case TCSETS:
+> + {
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + state->port->flags &= ~(SNX_UPF_SPD_HI |
+> + SNX_UPF_SPD_VHI |
+> + SNX_UPF_SPD_SHI |
+> + SNX_UPF_SPD_WARP);
+> + state->port->setserial_flag = SNX_SER_BAUD_NOTSETSER;
+> + snx_ser_update_termios(state);
+> + }
+> + break;
+> + }
+> +
+
+Why do you need your own implementation of these tty ioctl()'s ?
+The tty subsystem handles them on its own (using the callbacks for hw-
+specific operations)
+
ioctl functions support for SUNIX specific serial,parellel and cash drawer interface.
+
+> + case SNX_SER_DUMP_PORT_INFO:
+<snip>
+> + case SNX_SER_DUMP_DRIVER_VER:
+<snip>
+> + case SNX_COMM_GET_BOARD_CNT:
+<snip>
+> + case SNX_COMM_GET_BOARD_INFO:
+
+is it really necessary to introduce your own driver-specific ioctl() ?
+why not putting these things into debugfs or sysfs ?
+
ioctl functions support for SUNIX specific serial,
parellel and cash drawer interface.
+
+> + case SNX_GPIO_GET:
+<snip>
+> + case SNX_GPIO_SET:
+<snip>
+> + case SNX_GPIO_READ:
+<snip>
+> + case SNX_GPIO_WRITE:
+<snip>
+> + case SNX_GPIO_SET_DEFAULT:
+<snip>
+> + case SNX_GPIO_WRITE_DEFAULT:
+<snip>
+> + case SNX_GPIO_GET_INPUT_INVERT:
+<snip>
+> + case SNX_GPIO_SET_INPUT_INVERT:
+
+
+gpio stuff clearly doesn't belong into tty ioctl()s.
+
+that's what the gpio subsystem is there for - this provides the linux
+standard api for gpio access.
+
+> + case SNX_UART_GET_TYPE:
+
+what exactly is this for ?
+
+<snip>
+> + } else {
+> + //pr_err(CE_NOTE, "WARNING : incorrect port
+> + //number (port = %d)!",gb.uart_num);
+> + break;
+> + }
+> +
+> + switch (uart_type) {
+> + case 0: // RS-232
+
+yet another indention mismatch.
+and please remove dead code.
+
i will fix it.
+
+<snip>
+
+> + case SNX_UART_SET_TYPE: {
+
+what is this for ?
+
+<snip>
+
+> + case SNX_UART_SET_ACS:
+
+what is "ACS" ?
+
RS485 ACS Enable,Enable SUNIX UART Controller waiting transmit
data when receive data is going.
+
+> + {
+> + SNX_DRVR_UART_SET_ACS gb;
+> + struct sunix_board *sb = NULL;
+> + int ACSstate = 0;
+> + int targetConfigAddress = 0;
+> +
+> + memset(&gb, 0, sizeof(SNX_DRVR_UART_SET_ACS));
+> +
+> + if (copy_from_user(&gb, (void *)arg,
+> + (sizeof(SNX_DRVR_UART_SET_ACS))))
+> + ret = -EFAULT;
+> + else
+> + ret = 0;
+> +
+> + sb = &sunix_board_table[gb.board_id - 1];
+
+do we really need to access global variables here ?
+
i prefer to keep current format.
+
+<snip>
+
+> + if (tty->flags & (1 << TTY_IO_ERROR)) {
+> + ret = -EIO;
+> + goto out;
+> + }
+> +
+> + switch (cmd) {
+> + case TIOCMIWAIT:
+
+yet another idention mismatch.
+
i will fix it.
+
+<snip>
+
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + down(&state->sem);
+> +
+> + switch (cmd) {
+> + case TIOCSERGETLSR:
+> + ret = snx_ser_get_lsr_info(state, (unsigned int *)arg);
+> + break;
+> +
+> + default:
+> + {
+> + break;
+> + }
+> + }
+> +
+> + up(&state->sem);
+> + }
+
+even more indention mismatches.
+
i will fix it.
+
+<snip>
+
+> +static void snx_ser_set_termios(struct tty_struct *tty,
+> +struct SNXTERMIOS *old_termios)
+> +{
+> + struct snx_ser_state *state = NULL;
+> + unsigned long flags;
+> + unsigned int cflag = tty->termios.c_cflag;
+> + int line = SNX_SER_DEVNUM(tty);
+> +
+> + if (line >= SNX_SER_TOTAL_MAX)
+> + return;
+> +
+> + state = tty->driver_data;
+> +
+> +#define RELEVANT_IFLAG(iflag) ((iflag) & (IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK))
+
+please no #define's in the middle of a function.
+
i will fix it.
+
+<snip>
+
+> +static void snx_ser_update_timeout(struct snx_ser_port *port,
+> +unsigned int cflag, unsigned int baud)
+> +{
+> + unsigned int bits;
+> +
+> + switch (cflag & CSIZE) {
+> + case CS5:
+> + bits = 7;
+> + break;
+> +
+
+more indention mismatches.
+
i will fix it.
+
+<snip>
+
+> +static int snx_ser_open(struct tty_struct *tty, struct file *filp)
+> +{
+> + struct snx_ser_driver *drv =
+> + (struct snx_ser_driver *)tty->driver->driver_state;
+> +
+> + struct snx_ser_state *state = NULL;
+> + struct tty_port *tport = NULL;
+> +
+> + int retval = 0;
+> + int line = SNX_SER_DEVNUM(tty);
+> +
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + retval = -ENODEV;
+> +
+> + if (line >= SNX_SER_TOTAL_MAX)
+> + goto fail;
+> +
+> + state = snx_ser_get(drv, line);
+> +
+> + tport = &state->tport;
+> +
+> + if (IS_ERR(state)) {
+> + retval = PTR_ERR(state);
+> + goto fail;
+> + }
+> +
+> + if (!state)
+> + goto fail;
+> +
+> +
+> + state->port->suspended = 1;
+> + tty->driver_data = state;
+> +
+> + tport->low_latency = (state->port->flags &
+> + SNX_UPF_LOW_LATENCY) ? 1 : 0;
+> +
+> + state->info->tty = tty;
+> +
+> + tty_port_tty_set(tport, tty);
+> +
+> + if (tty_hung_up_p(filp)) {
+> + retval = -EAGAIN;
+> + state->count--;
+> + up(&state->sem);
+> + goto fail;
+> + }
+> +
+> + retval = snx_ser_startup(state, 0);
+> +
+> + if (retval == 0)
+> + retval = snx_ser_block_til_ready(filp, state);
+> +
+> + up(&state->sem);
+> +
+> + if (retval == 0 && !(state->info->flags &
+> + SNX_UIF_NORMAL_ACTIVE)) {
+> + state->info->flags |= SNX_UIF_NORMAL_ACTIVE;
+> +
+> + snx_ser_update_termios(state);
+> + }
+> +
+> + try_module_get(THIS_MODULE);
+
+why this ?
+
i will fix it.
+
+<snip>
+
+> +extern int sunix_ser_register_driver(struct snx_ser_driver *drv)
+> +{
+> + struct tty_driver *normal = NULL;
+> + int i;
+> + int ret = 0;
+> +
+> + drv->state = kmalloc(sizeof(struct snx_ser_state) * drv->nr,
+> + GFP_KERNEL);
+> +
+> + ret = -ENOMEM;
+> +
+> + if (!drv->state) {
+> + pr_err("SNX Error: Allocate memory fail !\n\n");
+> + goto out;
+> + }
+> +
+> + memset(drv->state, 0, sizeof(struct snx_ser_state) * drv->nr);
+> +
+> + for (i = 0; i < drv->nr; i++) {
+> + struct snx_ser_state *state = drv->state + i;
+> + struct tty_port *tport = &state->tport;
+> +
+> + tty_port_init(tport);
+
+does that really need to be globally in driver init, instead of in per
+port device->open (and use device's private data) ?
+
i prefer to keep current format.
+
+<snip>
+
+> +extern void sunix_ser_unregister_ports(struct snx_ser_driver *drv)
+
+why are these 'extern' ?!
+
functions definition in multiple .c files.
\ No newline at end of file
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Respond:add support for SUNIX Multi-I/O board
2019-03-15 10:06 [PATCH] Respond:add support for SUNIX Multi-I/O board Morris Ku
@ 2019-03-20 18:24 ` Enrico Weigelt, metux IT consult
2019-03-21 8:53 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-03-20 18:24 UTC (permalink / raw)
To: Morris Ku, gregkh; +Cc: morris_ku, linux-kernel, arnd
On 15.03.19 11:06, Morris Ku wrote:
Hi Morris,
> +Oh, is there really a hard restriction on the max number of boards ?
> +
> +driver support maximum 4 boards can be installed incombination
> +(up to 32 serial port and 2 parallel port)
Could somebody install more than 4 boards in a machine ?
Is there any problem w/ having this data allocated per board,
associating it to the corresponding struct device instance ?
You do know that - when using probe'able bus'es (eg. PCI) - the kernel
automatically creates device instances (per card) for you ? You just
have to register a driver w/ a table of the supported device IDs, and
the kernel will instantiate a device for you, finally calling the
probe() function to do the actual initialization.
For PCI, an easily understandable example could be pata_cmd640.c.
The other bus'es work in a similar way, even platform devices (those
which usually aren't on a probe'able bus) when using oftree or acpi.
> +> +static int snx_ser_port_total_cnt;
> +> +static int snx_par_port_total_cnt;
> +> +
> +> +int snx_board_count;
> +> +
> +> +static struct snx_ser_driver sunix_ser_reg = {
> +> + .dev_name = "ttySNX",
> +> + .major = 0,
> +> + .minor = 0,
> +> + .nr = (SNX_SER_TOTAL_MAX + 1),
> +> +};
> +
> +can't this be const ?
> +
> i prefer keep it in current format.
Why ? do you ever have to write into this struct ?
> +> +static irqreturn_t sunix_interrupt(int irq, void *dev_id)
> +> +{
> +> + struct sunix_ser_port *sp = NULL;
> +> + struct sunix_par_port *pp = NULL;
> +> + struct sunix_board *sb = NULL;
> +> + int i;
> +
> +> + int status = 0;
> +> +
> +> + int handled = IRQ_NONE;
> +> +
> +> + for (i = 0; i < SNX_BOARDS_MAX; i++) {
> +> +
> +> + if (dev_id == &(sunix_board_table[i])) {
> +> + sb = dev_id;
> +> + break;
> +> + }
> +> + }
> +
> +maybe put this index into the board data, so the loop on the global
> +array isn't needed ?
> +
> i prefer keep it in current format.
The code would be much cleaner if you'd just pass the pointer to the
corresponding struct sunix_board to (devm_)register_irq - this pointer
will then be passed as the second argument (dev_id) of the irq handler.
No need to scan the whole array, you'll directly get the right pointer.
> +<snip>
> +
> +> + if ((sb->ser_port > 0) && (sb->ser_isr != NULL)) {
> +> + sp = &sunix_ser_table[sb->ser_port_index];
> +
> +maybe put this pointer struct sunix_board so the lookup in the
> +global array isn't necessary ?
> +
> i prefer keep it in current format.
You do know that irq handlers should be as quick as possible ?
So, try to skip any unnecessary table scans.
> +<snip>
> +
> +> + if ((sb->par_port > 0) && (sb->par_isr != NULL)) {
> +> + pp = &sunix_par_table[sb->par_port_index];
> +> +
> +> + if (!pp)
> +> + status = 1;
> +> +
> +> + status = sb->par_isr(sb, pp);
> +> + }
> +> +
> +> + if (status != 0)
> +> + return handled;
> +> +
> +> + handled = IRQ_HANDLED;
> +> + return handled;
> +> +}
> +
> +btw: is there only one irq per board or one per port ?
> +
> only one irq per board.
That is very unfortunate. if there was one per port, you could register
one handler per port and let the kernel directly route to the right port
instance. (see above: let it directly pass the pointer to the port data)
Are there other devices on the board that share the same irq ?
> +if it already is a global, why not statically initialized ?
> +Why not doing this in snx_pci_probe() on per-board basis, and let device
> +registration be done by pci subsystem ?
> +
> i prefer keep it in current format.
See above: when probing on per board-basis, the pci subsystem already
handles most of the stuff for you, and the driver becomes smaller and
easier to understand.
> SUNIX has many differing cards,serail,parallel and
> multi-i/o(serial + parellel), therefore, we combaine to one driver.
But we have entirely different subsystems for the different device
types, which take care of the corresponding kernel-internal and userland
APIs. Therefore, for such a multi function device, you'd actually have
several drivers, registering in the corresponding subsystem. A serial
driver registering in the serial subsystem, a gpio driver in the gpio
subsystem, etc ...
This is important! This is how hardware is used on Linux: we have one
API per device class, not hundreds, one for each device type or
manufacturer. This is exactly one of the major aspects that make Linux
so extremly universal and easily maintainable.
Nobody seriously wants to have hardware/manufacturer specific APIs.
> +<snip>
> +
> +> + if ((gpio_ch_cnt == 0x00) && (gpio_card_type == 0x01))
> +> + gpio_ch_cnt = 6;
> +> + else if ((gpio_ch_cnt == 0x00) && (gpio_card_type == 0x02))
> +> + gpio_ch_cnt = 8;
> +> + else if (gpio_ch_cnt == 0x01)
> +> + gpio_ch_cnt = 16;
> +> + else if (gpio_ch_cnt == 0x02)
> +> + gpio_ch_cnt = 32;
> +> +
> +
> +gpio devices have their own subsystem -> therefore: write a separate
> +gpio driver for that.
> +
> ioctl functions support for SUNIX specific serial,parellel and cash drawer
> interface.
As said: nobody likes hardware specific APIs.
> driver support maximum 4 boards can be installed incombination
> (up to 32 serial port and 2 parallel port)
Why this limitation ? Why just 4, not 8 or 16 ? Sounds pretty arbitrary.
> +> +static void __exit snx_exit(void)
> +> +{
> +> + if (snx_par_port_total_cnt > 0) {
> +> + sunix_par_lp_exit();
> +> + sunix_par_ppdev_exit();
> +> + sunix_par_parport_exit();
> +> + }
> +
> +Let the cleanup be done by the individual driver's release functions,
> +and down forget to set .owner correctly - that way, the kernel won't
> +even allow unloading the module, as long as it's in use. then, you'd
> +probably don't need much more than just removing unregistering the
> +drivers here.
> +
> i prefer keep it in current format.
This isn't quite how the LDM works. You should be aware that drivers can
be unbound and rebound by the operator any time, even if the card
remains physically plugged (eg. important for things like PCI passthrough.)
Your driver doesn't support that (smells even like unpredictable
behaviour when somebody tries it), without giving any good reason.
> +> +static int EEPROMWriteData(int targetConfigAddress, int address, int data)
> +
> +why do you use int instead of void* for addresses ?
> +oh, and shouldn't it be __iomem ?
> +
> +targetConfigAddress for PCI (bar)base address register.
No, I was asking about the __iomem qualifier.
https://stackoverflow.com/questions/19100536/what-is-the-use-of-iomem-in-linux-while-writing-device-drivers
> +why this explicit check for CAP_SYS_ADMIN ?
> +
> check users with admin rights
Why exactly is that extra check needed ?
Are you aware of how filesystem permissions on device nodes work ?
> +why is userland allowed to change irq, port, etc, if that's probed
> +from pci anyways ?
> +
> +
> Since changing the 'type' of the port changes its resource
> allocations, we should treat type changes the same as
> IO port changes.
What exactly do you mean by 'type' ?
Anyways, userland shouldn't have access to such settings.
You should be away that it's very common to give certain unprivileged
users access to such devices, just via filesystem permissions (that's
eg. what the group 'dialout' is for). Therefore: don't allow any
potentially harmful operations here.
> +> +static int snx_ser_ioctl(struct tty_struct *tty,
> +> +unsigned int cmd, unsigned long arg)
> +> +{
> +<snip>
> +> + case TIOCSSERIAL:
> +> + {
> +> + if (line < SNX_SER_TOTAL_MAX) {
> +> + state->port->setserial_flag = SNX_SER_BAUD_SETSERIAL;
> +> + ret = snx_ser_set_info(state,
> +> + (struct serial_struct *)arg);
> +> + }
> +> + break;
> +> + }
> +> +
> +> +
> +> + case TCSETS:
> +> + {
> +> + if (line < SNX_SER_TOTAL_MAX) {
> +> + state->port->flags &= ~(SNX_UPF_SPD_HI |
> +> + SNX_UPF_SPD_VHI |
> +> + SNX_UPF_SPD_SHI |
> +> + SNX_UPF_SPD_WARP);
> +> + state->port->setserial_flag = SNX_SER_BAUD_NOTSETSER;
> +> + snx_ser_update_termios(state);
> +> + }
> +> + break;
> +> + }
> +> +
> +
> +Why do you need your own implementation of these tty ioctl()'s ?
> +The tty subsystem handles them on its own (using the callbacks for hw-
> +specific operations)
> +
> ioctl functions support for SUNIX specific serial,parellel and cash drawer interface.
As already set: the serial subsystem already handles that for you.
Don't reimplement existing standard functionality yourself, in your
special own way.
> +> + case SNX_SER_DUMP_PORT_INFO:
> +<snip>
> +> + case SNX_SER_DUMP_DRIVER_VER:
> +<snip>
> +> + case SNX_COMM_GET_BOARD_CNT:
> +<snip>
> +> + case SNX_COMM_GET_BOARD_INFO:
> +
> +is it really necessary to introduce your own driver-specific ioctl() ?
> +why not putting these things into debugfs or sysfs ?
> +
> ioctl functions support for SUNIX specific serial,
> parellel and cash drawer interface.
As already said: we have our own systems for that kind of functionality.
Don't reimplement existing standard functionality yourself, in your
special own way.
> +> + case SNX_UART_SET_TYPE: {
> +
> +what is this for ?
> +
> +<snip>
> +
> +> + case SNX_UART_SET_ACS:
> +
> +what is "ACS" ?
> +
> RS485 ACS Enable,Enable SUNIX UART Controller waiting transmit
> data when receive data is going.
IMHO serial subsystem should already have standard functionality for
that. Greg (the serial subsystem maintainer) probably can tell you
more about that.
> +> +extern int sunix_ser_register_driver(struct snx_ser_driver *drv)
> +> +{
> +> + struct tty_driver *normal = NULL;
> +> + int i;
> +> + int ret = 0;
> +> +
> +> + drv->state = kmalloc(sizeof(struct snx_ser_state) * drv->nr,
> +> + GFP_KERNEL);
> +> +
> +> + ret = -ENOMEM;
> +> +
> +> + if (!drv->state) {
> +> + pr_err("SNX Error: Allocate memory fail !\n\n");
> +> + goto out;
> +> + }
> +> +
> +> + memset(drv->state, 0, sizeof(struct snx_ser_state) * drv->nr);
> +> +
> +> + for (i = 0; i < drv->nr; i++) {
> +> + struct snx_ser_state *state = drv->state + i;
> +> + struct tty_port *tport = &state->tport;
> +> +
> +> + tty_port_init(tport);
> +
> +does that really need to be globally in driver init, instead of in per
> +port device->open (and use device's private data) ?
> +
> i prefer to keep current format.
The standard Linux method is to do that in the corresponding probe()
function.
> +> +extern void sunix_ser_unregister_ports(struct snx_ser_driver *drv)
> +
> +why are these 'extern' ?!
> +
> functions definition in multiple .c files.
for functions, rototype w/ extern is sufficient.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Respond:add support for SUNIX Multi-I/O board
2019-03-20 18:24 ` Enrico Weigelt, metux IT consult
@ 2019-03-21 8:53 ` Arnd Bergmann
2019-03-22 18:26 ` Enrico Weigelt, metux IT consult
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2019-03-21 8:53 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: Morris Ku, gregkh, morris_ku, Linux Kernel Mailing List
On Wed, Mar 20, 2019 at 7:24 PM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 15.03.19 11:06, Morris Ku wrote:
> > +Oh, is there really a hard restriction on the max number of boards ?
> > +
> > +driver support maximum 4 boards can be installed incombination
> > +(up to 32 serial port and 2 parallel port)
>
> Could somebody install more than 4 boards in a machine ?
>
> Is there any problem w/ having this data allocated per board,
> associating it to the corresponding struct device instance ?
>
> You do know that - when using probe'able bus'es (eg. PCI) - the kernel
> automatically creates device instances (per card) for you ? You just
> have to register a driver w/ a table of the supported device IDs, and
> the kernel will instantiate a device for you, finally calling the
> probe() function to do the actual initialization.
>
> For PCI, an easily understandable example could be pata_cmd640.c.
> The other bus'es work in a similar way, even platform devices (those
> which usually aren't on a probe'able bus) when using oftree or acpi.
I think serial port drivers typically have a limit on the number of
ports, so we likely need the same here. However the number of
boards should not be limited in the mfd driver that just exports
the serial ports to the serial driver.
Similarly, there should be no limit for the parallel port driver and
the gpio driver.
> > +> +static int snx_ser_port_total_cnt;
> > +> +static int snx_par_port_total_cnt;
> > +> +
> > +> +int snx_board_count;
> > +> +
> > +> +static struct snx_ser_driver sunix_ser_reg = {
> > +> + .dev_name = "ttySNX",
> > +> + .major = 0,
> > +> + .minor = 0,
> > +> + .nr = (SNX_SER_TOTAL_MAX + 1),
> > +> +};
> > +
> > +can't this be const ?
> > +
> > i prefer keep it in current format.
>
> Why ? do you ever have to write into this struct ?
I think this will just go away after the rewrite into a proper
layered driver, so it doesn't matter.
> > +<snip>
> > +
> > +> + if ((sb->par_port > 0) && (sb->par_isr != NULL)) {
> > +> + pp = &sunix_par_table[sb->par_port_index];
> > +> +
> > +> + if (!pp)
> > +> + status = 1;
> > +> +
> > +> + status = sb->par_isr(sb, pp);
> > +> + }
> > +> +
> > +> + if (status != 0)
> > +> + return handled;
> > +> +
> > +> + handled = IRQ_HANDLED;
> > +> + return handled;
> > +> +}
> > +
> > +btw: is there only one irq per board or one per port ?
> > +
> > only one irq per board.
>
> That is very unfortunate. if there was one per port, you could register
> one handler per port and let the kernel directly route to the right port
> instance. (see above: let it directly pass the pointer to the port data)
>
> Are there other devices on the board that share the same irq ?
The best way I can think of to do this would be to have a nested
irqchip in the mfd driver that handles the per-board interrupt and
provides a unique irq number to the individual port drivers.
This makes a very simple irq handler in the port driver, and keeps
the multiplexing where it belongs.
> > +if it already is a global, why not statically initialized ?
> > +Why not doing this in snx_pci_probe() on per-board basis, and let device
> > +registration be done by pci subsystem ?
> > +
> > i prefer keep it in current format.
>
> See above: when probing on per board-basis, the pci subsystem already
> handles most of the stuff for you, and the driver becomes smaller and
> easier to understand.
Agreed, this is only correct way to do it.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Respond:add support for SUNIX Multi-I/O board
2019-03-21 8:53 ` Arnd Bergmann
@ 2019-03-22 18:26 ` Enrico Weigelt, metux IT consult
0 siblings, 0 replies; 6+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-03-22 18:26 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Morris Ku, gregkh, morris_ku, Linux Kernel Mailing List
On 21.03.19 09:53, Arnd Bergmann wrote:
> I think serial port drivers typically have a limit on the number of
> ports, so we likely need the same here.
Except for earlycon (haven't had a closer look at it yet), I don't see
any hard reason for that (maybe Greg could give us more insight here).
I seriously doubt that these boards could be relevant for earlycon.
(hard to find a machine that needs serial earlycon while not having some
uart on-board ;-))
> However the number of boards should not be limited in the mfd driver
> that just exports the serial ports to the serial driver.
Exactly. And there should be any global arrays for these boards, where
every access runs through. Or even worse: looping through it, to find
the right entry, within ISRs.
>>> +> +static int snx_ser_port_total_cnt;
>>> +> +static int snx_par_port_total_cnt;
>>> +> +
>>> +> +int snx_board_count;
>>> +> +
>>> +> +static struct snx_ser_driver sunix_ser_reg = {
>>> +> + .dev_name = "ttySNX",
>>> +> + .major = 0,
>>> +> + .minor = 0,
>>> +> + .nr = (SNX_SER_TOTAL_MAX + 1),
>>> +> +};
>>> +
>>> +can't this be const ?
>>> +
>>> i prefer keep it in current format.
>>
>> Why ? do you ever have to write into this struct ?
>
> I think this will just go away after the rewrite into a proper
> layered driver, so it doesn't matter.
That's exactly the point, which I've been repeating over and over again.
By the way, one more weird thing that slipped trough my previous
reviews: he's redefining standard structs and typecasting them.
Actually, he copy+paste'd large parts of the serial and parport
subsystems.
> The best way I can think of to do this would be to have a nested
> irqchip in the mfd driver that handles the per-board interrupt and
> provides a unique irq number to the individual port drivers.
> This makes a very simple irq handler in the port driver, and keeps
> the multiplexing where it belongs.
Exactly. That's the way it works w/ lot's of other composite devices.
(I recall some exotic daq cards that work similar way)
@Morris: please try to find a fitting generic, configurable irqchip
driver first. If you don't find one, talk to us and give us a detailed
explaination how that part of your cards actually works, so we could
write some generic driver. I might have some customer specific devices
that could also benefit from that.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Respond:add support for SUNIX Multi-I/O board
2019-03-15 10:10 Morris Ku
@ 2019-03-27 16:38 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2019-03-27 16:38 UTC (permalink / raw)
To: Morris Ku; +Cc: lkml, morris_ku, linux-kernel, arnd
On Fri, Mar 15, 2019 at 06:10:59PM +0800, Morris Ku wrote:
> +driver support maximum 4 boards can be installed incombination
> +(up to 32 serial port and 2 parallel port)
> +
> +And do we really need a global list of them ? (instead of just having
> +all per-board / per-port data in a per-board / per-port driver instance)
> +
> +
> i prefer to keep current format.
No, this needs to be fixed, you can not use static arrays anymore for
stuff like this.
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Respond:add support for SUNIX Multi-I/O board
@ 2019-03-15 10:10 Morris Ku
2019-03-27 16:38 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Morris Ku @ 2019-03-15 10:10 UTC (permalink / raw)
To: lkml; +Cc: gregkh, morris_ku, linux-kernel, arnd, Morris Ku
Hi Enrico,
Thanks for review, my replies are inline:
Signed-off-by: Morris Ku <saumah@gmail.com>
---
+From 5b1c4c8f7d91661a27c88b980c42b768e4cb7606 Mon Sep 17 00:00:00 2001
+From: Morris Ku <saumah@gmail.com>
+Date: Tue, 26 Feb 2019 17:11:48 +0800
+Subject: [PATCH 6/6] add support for SUNIX Multi-I/O board
+
+
+<snip>
+
+> diff --git a/char/snx/snx_main.c b/char/snx/snx_main.c
+
+if it's a serial card, shouldn't it go to drivers/tty/serial/snx/ ?
+
i will move to drivers/mfd (multi function devices)
+
+
+<snip>
+
+> +char
+[SNX_SER_PORT_MAX_UART][10] = {
+> + {"UNKNOWN"},
+> + {"SUN1889"},
+> + {"SUN1699"},
+> + {"SUNMATX"},
+> + {"SUN1999"}
+> +};
+> +
+> +char snx_par_ic_table[SNX_PAR_PORT_MAX_UART][10] = {
+> + {"UNKNOWN"},
+> + {"SUN1888"},
+> + {"SUN1689"},
+> + {"SUNMATX"},
+> + {"SUN1999"}
+> +};
+> +
+> +char snx_port_remap[2][10] = {
+> + {"NON-REMAP"},
+> + {"REMAP"}
+> +};
+
+can't these be const static ?
+
i will fix it.
+
+can we have a bit more consistent formatting (eg. all those comments
+w/ same indention) ?
+
i prefer to keep it in current format.
+
+> + SUN1999_BOARD_5008A,
+> +
+
+<snip>
+
+> +static struct pci_device_id sunix_pci_board_id[] = {
+> +// golden-serial
+
+can't this be const ?
+
i will fix it.
+
+(maybe even __initconst)
+I'd really prefer separate port types - that go into separate
+subsystems, in separate modules (those can share code in a common
+module, if desired)
+
+Oh, is there really a hard restriction on the max number of boards ?
+
+driver support maximum 4 boards can be installed incombination
+(up to 32 serial port and 2 parallel port)
+
+And do we really need a global list of them ? (instead of just having
+all per-board / per-port data in a per-board / per-port driver instance)
+
+
i prefer to keep current format.
+
+
+> +static int snx_ser_port_total_cnt;
+> +static int snx_par_port_total_cnt;
+> +
+> +int snx_board_count;
+> +
+> +static struct snx_ser_driver sunix_ser_reg = {
+> + .dev_name = "ttySNX",
+> + .major = 0,
+> + .minor = 0,
+> + .nr = (SNX_SER_TOTAL_MAX + 1),
+> +};
+
+can't this be const ?
+
i prefer keep it in current format.
+
+> +static irqreturn_t sunix_interrupt(int irq, void *dev_id)
+> +{
+> + struct sunix_ser_port *sp = NULL;
+> + struct sunix_par_port *pp = NULL;
+> + struct sunix_board *sb = NULL;
+> + int i;
+
+> + int status = 0;
+> +
+> + int handled = IRQ_NONE;
+> +
+> + for (i = 0; i < SNX_BOARDS_MAX; i++) {
+> +
+> + if (dev_id == &(sunix_board_table[i])) {
+> + sb = dev_id;
+> + break;
+> + }
+> + }
+
+maybe put this index into the board data, so the loop on the global
+array isn't needed ?
+
i prefer keep it in current format.
+
+<snip>
+
+> + if ((sb->ser_port > 0) && (sb->ser_isr != NULL)) {
+> + sp = &sunix_ser_table[sb->ser_port_index];
+
+maybe put this pointer struct sunix_board so the lookup in the
+global array isn't necessary ?
+
i prefer keep it in current format.
+
+<snip>
+
+> + if ((sb->par_port > 0) && (sb->par_isr != NULL)) {
+> + pp = &sunix_par_table[sb->par_port_index];
+> +
+> + if (!pp)
+> + status = 1;
+> +
+> + status = sb->par_isr(sb, pp);
+> + }
+> +
+> + if (status != 0)
+> + return handled;
+> +
+> + handled = IRQ_HANDLED;
+> + return handled;
+> +}
+
+btw: is there only one irq per board or one per port ?
+
only one irq per board.
+
+> +static int snx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+> +{
+> + return 0;
+
+shouldn't probe check whether the device is actually there and then do
+the initialization ?
+
i will fix it.
+
+<snip>
+
+> +static int snx_resume_port_termios(struct snx_ser_info *info)
+> +{
+> + struct snx_ser_state *state = NULL;
+> + struct tty_struct *tty = info->tty;
+> +
+> + state = tty->driver_data;
+> + snx_set_port_termios(state);
+> +
+> + return 0;
+> +}
+> +
+> +
+
+do we need multiple newlines here ?
+
i will fix it.
+
+<snip>
+
+> +static int snx_resume_one(struct pci_dev *pdev)
+> +{
+> + struct sunix_board *sb = pci_get_drvdata(pdev);
+> + struct sunix_ser_port *sp = NULL;
+> + int j;
+> +
+> + if (sb == NULL)
+> + return 0;
+> +
+> + for (j = 0; j < sb->ser_port; j++) {
+> + sp = &sunix_ser_table[j];
+
+can't the pointers to the serial ports be in struct sunix_board instead
+of an global array ?
+
+if it already is a global, why not statically initialized ?
+Why not doing this in snx_pci_probe() on per-board basis, and let device
+registration be done by pci subsystem ?
+
i prefer keep it in current format.
+
+> + // search sun1999 muti I/O board
+> + pdev = NULL;
+> + tablecnt = 0;
+> + sub_device_id = 0;
+> + status = 0;
+> + device_part_number = 0;
+> + bar3_base_add = 0;
+> + bar3_Byte5 = 0;
+> + bar3_Byte6 = 0;
+> + bar3_Byte7 = 0;
+> + oem_id = 0;
+> + uart_type = 0;
+> + gpio_type = 0;
+> + gpio_card_type = 0;
+> + gpio_ch_cnt = 0;
+
+completely different boards handled by one big driver ?
+why not splitting it up into multiple drivers ?
+
SUNIX has many differing cards,serail,parallel and
multi-i/o(serial + parellel), therefore, we combaine to one driver.
+
+<snip>
+
+> + if ((gpio_ch_cnt == 0x00) && (gpio_card_type == 0x01))
+> + gpio_ch_cnt = 6;
+> + else if ((gpio_ch_cnt == 0x00) && (gpio_card_type == 0x02))
+> + gpio_ch_cnt = 8;
+> + else if (gpio_ch_cnt == 0x01)
+> + gpio_ch_cnt = 16;
+> + else if (gpio_ch_cnt == 0x02)
+> + gpio_ch_cnt = 32;
+> +
+
+gpio devices have their own subsystem -> therefore: write a separate
+gpio driver for that.
+
ioctl functions support for SUNIX specific serial,parellel and cash drawer
interface.
+
+<snip>
+why that limitation ? why does this have to be a global array at all ?
+
driver support maximum 4 boards can be installed incombination
(up to 32 serial port and 2 parallel port)
+
+> +static int sunix_assign_resource(void)
+> +{
+> + struct sunix_board *sb = NULL;
+> + struct sunix_ser_port *sp = NULL;
+> + struct sunix_par_port *pp = NULL;
+> +
+> + int status = 0;
+> + int i;
+> + int j;
+> + int k;
+> + int ser_n;
+> + int ser_port_index = 0;
+> +
+> + memset(sunix_ser_table, 0, (SNX_SER_TOTAL_MAX + 1) *
+> + sizeof(struct sunix_ser_port));
+> +
+> + memset(sunix_par_table, 0, (SNX_PAR_TOTAL_MAX) *
+> + sizeof(struct sunix_par_port));
+
+why this w/ global variables, instead of on per-device basis on device-
+private data ?
+
i prefer keep it in current format.
+
+<snip>
+
+> +static int sunix_ser_port_table_init(void)
+> +{
+
+same here.
+
+<snip>
+
+> +static int sunix_par_port_table_init(void)
+> +{
+
+same here
+
+> +int sunix_register_irq(void)
+> +{
+
+same here.
+
+<snip>
+why not doing those initializations on per-device basis, in the
+corresponding probe() function ?
+use devm_*() functions, which automatically releases resource when a
+device is deleted, so explicit release isn't needed anymore.
+
i will fix it.
+
+> +static void __exit snx_exit(void)
+> +{
+> + if (snx_par_port_total_cnt > 0) {
+> + sunix_par_lp_exit();
+> + sunix_par_ppdev_exit();
+> + sunix_par_parport_exit();
+> + }
+
+Let the cleanup be done by the individual driver's release functions,
+and down forget to set .owner correctly - that way, the kernel won't
+even allow unloading the module, as long as it's in use. then, you'd
+probably don't need much more than just removing unregistering the
+drivers here.
+
i prefer keep it in current format.
+
+> diff --git a/char/snx/snx_serial.c b/char/snx/snx_serial.c
+> new file mode 100644
+> index 00000000..fdac4c90
+> --- /dev/null
+> +++ b/char/snx/snx_serial.c
+> @@ -0,0 +1,4263 @@
+> +// SPDX-License-Identifier: GPL-2.0
+> +#include "snx_common.h"
+> +#include "driver_extd.h"
+> +
+> +#define SNX_ioctl_DBG 0
+
+what exactly is that ioctl for ?
+or more to the point: why are you introducing a driver specific iotctl ?
+
i will drop it.
+
+> +#define EEPROM_ACCESS_DELAY_COUNT 100000
+> +
+> +static DEFINE_SEMAPHORE(ser_port_sem);
+> +
+> +#define SNX_HIGH_BITS_OFFSET ((sizeof(long)-sizeof(int))*8)
+> +#define sunix_ser_users(state) \
+> +((state)->count + ((state)->info ? (state)->info->blocked_open : 0))
+> +
+> +static struct tty_port snx_service_port;
+
+why has this to be global, instead of in per-device private data ?
+
I am not sure what you mean ?
+
+<snip>
+
+> +static _INLINE_ void snx_ser_handle_cts_change(
+> + struct snx_ser_port *, unsigned int);
+
+Where's this strange "_INLINE_" coming from ?
+
i will fix it.
+
+<snip>
+
+> +//extern void snx_ser_change_speed(
+> + //struct snx_ser_state *state, struct SNXTERMIOS *old_termios);
+
+please no dead/commented-out code.
+
i will drop it.
+
+> +//extern int sunix_ser_interrupt(
+> + //struct sunix_board *, struct sunix_ser_port *first_sp);
+
+same here
+
i will drop it.
+
+> +//extern int sunix_ser_register_ports(struct snx_ser_driver *drv);
+> +//extern void sunix_ser_unregister_ports(struct snx_ser_driver *drv);
+> +//extern int sunix_ser_register_driver(struct snx_ser_driver *drv);
+> +//extern void sunix_ser_unregister_driver(struct snx_ser_driver *drv);
+
+same here
+
i will drop it.
+
+> +static unsigned char READ_INTERRUPT_VECTOR_BYTE(struct sunix_ser_port *sp)
+
+i'd recommend no caps in function names.
+
i will fix it.
+
+> + if (var == 0x04) {
+> + vet4 = inb(local_vector + 0xb0);
+> + vet4 <<= 12;
+> + }
+> +
+> + data = (vet1 | vet2 | vet3 | vet4);
+> +
+> + return data;
+> + }
+> + return 0;
+> +}
+> +
+> +
+> +
+
+remove excess newlines
+
i will fix it.
+
+<snip>
+
+> +static int EEPROMWriteData(int targetConfigAddress, int address, int data)
+
+why do you use int instead of void* for addresses ?
+oh, and shouldn't it be __iomem ?
+
+targetConfigAddress for PCI (bar)base address register.
+
+<snip>
+
+> + Error = inb(targetConfigAddress + 0x08) & 0x04;
+
+<snip>
+
+> +static void snx_ser_start(struct tty_struct *tty)
+> +{
+> + int line = SNX_SER_DEVNUM(tty);
+> +
+> + if (line >= SNX_SER_TOTAL_MAX)
+> + return;
+> +
+> + //spin_lock_irqsave(&port->lock, flags);
+> + __snx_ser_start(tty);
+> + //spin_unlock_irqrestore(&port->lock, flags);
+
+why are the spinlock calls commented out ?
+
i wiil fix it.
+
+<nsip>
+
+> + tasklet_kill(&info->tlet);
+
+what exactly is the tasklet needed for ?
+
i will fix it.
+
+> + if (!capable(CAP_SYS_ADMIN)) {
+> + retval = -EPERM;
+
+why this explicit check for CAP_SYS_ADMIN ?
+
check users with admin rights
+
+> + if (change_irq ||
+
+indention mismatch.
+
i will fix it.
+
+> + change_port ||
+
+why is userland allowed to change irq, port, etc, if that's probed
+from pci anyways ?
+
+
Since changing the 'type' of the port changes its resource
allocations, we should treat type changes the same as
IO port changes.
+
+>
+
+ (new_serial.baud_base != port->uartclk / 16) ||
+> + (close_delay != state->close_delay) ||
+> + (closing_wait != state->closing_wait) ||
+> + (new_serial.xmit_fifo_size != port->fifosize) ||
+> + (((new_flags ^ old_flags) & ~SNX_UPF_USR_MASK) != 0)) {
+> + goto exit;
+> + }
+> +
+> + port->flags = ((port->flags & ~SNX_UPF_USR_MASK) |
+> + (new_flags & SNX_UPF_USR_MASK));
+> + port->custom_divisor = new_serial.custom_divisor;
+> + goto check_and_exit;
+> + }
+
+<snip>
+
+> +static int snx_ser_ioctl(struct tty_struct *tty,
+> +unsigned int cmd, unsigned long arg)
+> +{
+<snip>
+> + case TIOCSSERIAL:
+> + {
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + state->port->setserial_flag = SNX_SER_BAUD_SETSERIAL;
+> + ret = snx_ser_set_info(state,
+> + (struct serial_struct *)arg);
+> + }
+> + break;
+> + }
+> +
+> +
+> + case TCSETS:
+> + {
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + state->port->flags &= ~(SNX_UPF_SPD_HI |
+> + SNX_UPF_SPD_VHI |
+> + SNX_UPF_SPD_SHI |
+> + SNX_UPF_SPD_WARP);
+> + state->port->setserial_flag = SNX_SER_BAUD_NOTSETSER;
+> + snx_ser_update_termios(state);
+> + }
+> + break;
+> + }
+> +
+
+Why do you need your own implementation of these tty ioctl()'s ?
+The tty subsystem handles them on its own (using the callbacks for hw-
+specific operations)
+
ioctl functions support for SUNIX specific serial,parellel and cash drawer interface.
+
+> + case SNX_SER_DUMP_PORT_INFO:
+<snip>
+> + case SNX_SER_DUMP_DRIVER_VER:
+<snip>
+> + case SNX_COMM_GET_BOARD_CNT:
+<snip>
+> + case SNX_COMM_GET_BOARD_INFO:
+
+is it really necessary to introduce your own driver-specific ioctl() ?
+why not putting these things into debugfs or sysfs ?
+
ioctl functions support for SUNIX specific serial,
parellel and cash drawer interface.
+
+> + case SNX_GPIO_GET:
+<snip>
+> + case SNX_GPIO_SET:
+<snip>
+> + case SNX_GPIO_READ:
+<snip>
+> + case SNX_GPIO_WRITE:
+<snip>
+> + case SNX_GPIO_SET_DEFAULT:
+<snip>
+> + case SNX_GPIO_WRITE_DEFAULT:
+<snip>
+> + case SNX_GPIO_GET_INPUT_INVERT:
+<snip>
+> + case SNX_GPIO_SET_INPUT_INVERT:
+
+
+gpio stuff clearly doesn't belong into tty ioctl()s.
+
+that's what the gpio subsystem is there for - this provides the linux
+standard api for gpio access.
+
+> + case SNX_UART_GET_TYPE:
+
+what exactly is this for ?
+
+<snip>
+> + } else {
+> + //pr_err(CE_NOTE, "WARNING : incorrect port
+> + //number (port = %d)!",gb.uart_num);
+> + break;
+> + }
+> +
+> + switch (uart_type) {
+> + case 0: // RS-232
+
+yet another indention mismatch.
+and please remove dead code.
+
i will fix it.
+
+<snip>
+
+> + case SNX_UART_SET_TYPE: {
+
+what is this for ?
+
+<snip>
+
+> + case SNX_UART_SET_ACS:
+
+what is "ACS" ?
+
RS485 ACS Enable,Enable SUNIX UART Controller waiting transmit
data when receive data is going.
+
+> + {
+> + SNX_DRVR_UART_SET_ACS gb;
+> + struct sunix_board *sb = NULL;
+> + int ACSstate = 0;
+> + int targetConfigAddress = 0;
+> +
+> + memset(&gb, 0, sizeof(SNX_DRVR_UART_SET_ACS));
+> +
+> + if (copy_from_user(&gb, (void *)arg,
+> + (sizeof(SNX_DRVR_UART_SET_ACS))))
+> + ret = -EFAULT;
+> + else
+> + ret = 0;
+> +
+> + sb = &sunix_board_table[gb.board_id - 1];
+
+do we really need to access global variables here ?
+
i prefer to keep current format.
+
+<snip>
+
+> + if (tty->flags & (1 << TTY_IO_ERROR)) {
+> + ret = -EIO;
+> + goto out;
+> + }
+> +
+> + switch (cmd) {
+> + case TIOCMIWAIT:
+
+yet another idention mismatch.
+
i will fix it.
+
+<snip>
+
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + down(&state->sem);
+> +
+> + switch (cmd) {
+> + case TIOCSERGETLSR:
+> + ret = snx_ser_get_lsr_info(state, (unsigned int *)arg);
+> + break;
+> +
+> + default:
+> + {
+> + break;
+> + }
+> + }
+> +
+> + up(&state->sem);
+> + }
+
+even more indention mismatches.
+
i will fix it.
+
+<snip>
+
+> +static void snx_ser_set_termios(struct tty_struct *tty,
+> +struct SNXTERMIOS *old_termios)
+> +{
+> + struct snx_ser_state *state = NULL;
+> + unsigned long flags;
+> + unsigned int cflag = tty->termios.c_cflag;
+> + int line = SNX_SER_DEVNUM(tty);
+> +
+> + if (line >= SNX_SER_TOTAL_MAX)
+> + return;
+> +
+> + state = tty->driver_data;
+> +
+> +#define RELEVANT_IFLAG(iflag) ((iflag) & (IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK))
+
+please no #define's in the middle of a function.
+
i will fix it.
+
+<snip>
+
+> +static void snx_ser_update_timeout(struct snx_ser_port *port,
+> +unsigned int cflag, unsigned int baud)
+> +{
+> + unsigned int bits;
+> +
+> + switch (cflag & CSIZE) {
+> + case CS5:
+> + bits = 7;
+> + break;
+> +
+
+more indention mismatches.
+
i will fix it.
+
+<snip>
+
+> +static int snx_ser_open(struct tty_struct *tty, struct file *filp)
+> +{
+> + struct snx_ser_driver *drv =
+> + (struct snx_ser_driver *)tty->driver->driver_state;
+> +
+> + struct snx_ser_state *state = NULL;
+> + struct tty_port *tport = NULL;
+> +
+> + int retval = 0;
+> + int line = SNX_SER_DEVNUM(tty);
+> +
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + retval = -ENODEV;
+> +
+> + if (line >= SNX_SER_TOTAL_MAX)
+> + goto fail;
+> +
+> + state = snx_ser_get(drv, line);
+> +
+> + tport = &state->tport;
+> +
+> + if (IS_ERR(state)) {
+> + retval = PTR_ERR(state);
+> + goto fail;
+> + }
+> +
+> + if (!state)
+> + goto fail;
+> +
+> +
+> + state->port->suspended = 1;
+> + tty->driver_data = state;
+> +
+> + tport->low_latency = (state->port->flags &
+> + SNX_UPF_LOW_LATENCY) ? 1 : 0;
+> +
+> + state->info->tty = tty;
+> +
+> + tty_port_tty_set(tport, tty);
+> +
+> + if (tty_hung_up_p(filp)) {
+> + retval = -EAGAIN;
+> + state->count--;
+> + up(&state->sem);
+> + goto fail;
+> + }
+> +
+> + retval = snx_ser_startup(state, 0);
+> +
+> + if (retval == 0)
+> + retval = snx_ser_block_til_ready(filp, state);
+> +
+> + up(&state->sem);
+> +
+> + if (retval == 0 && !(state->info->flags &
+> + SNX_UIF_NORMAL_ACTIVE)) {
+> + state->info->flags |= SNX_UIF_NORMAL_ACTIVE;
+> +
+> + snx_ser_update_termios(state);
+> + }
+> +
+> + try_module_get(THIS_MODULE);
+
+why this ?
+
i will fix it.
+
+<snip>
+
+> +extern int sunix_ser_register_driver(struct snx_ser_driver *drv)
+> +{
+> + struct tty_driver *normal = NULL;
+> + int i;
+> + int ret = 0;
+> +
+> + drv->state = kmalloc(sizeof(struct snx_ser_state) * drv->nr,
+> + GFP_KERNEL);
+> +
+> + ret = -ENOMEM;
+> +
+> + if (!drv->state) {
+> + pr_err("SNX Error: Allocate memory fail !\n\n");
+> + goto out;
+> + }
+> +
+> + memset(drv->state, 0, sizeof(struct snx_ser_state) * drv->nr);
+> +
+> + for (i = 0; i < drv->nr; i++) {
+> + struct snx_ser_state *state = drv->state + i;
+> + struct tty_port *tport = &state->tport;
+> +
+> + tty_port_init(tport);
+
+does that really need to be globally in driver init, instead of in per
+port device->open (and use device's private data) ?
+
i prefer to keep current format.
+
+<snip>
+
+> +extern void sunix_ser_unregister_ports(struct snx_ser_driver *drv)
+
+why are these 'extern' ?!
+
functions definition in multiple .c files.
\ No newline at end of file
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-27 18:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-15 10:06 [PATCH] Respond:add support for SUNIX Multi-I/O board Morris Ku
2019-03-20 18:24 ` Enrico Weigelt, metux IT consult
2019-03-21 8:53 ` Arnd Bergmann
2019-03-22 18:26 ` Enrico Weigelt, metux IT consult
2019-03-15 10:10 Morris Ku
2019-03-27 16:38 ` Greg KH
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).