On 08.03.19 13:34, Morris Ku wrote: Hi,
<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/ ? <snip> > +char snx_ser_ic_table[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 ? > + > +enum{ > +// golden-serial > + GOLDEN_BOARD_TEST = 0, > + > +// matrix-serial > + MATRIX_BOARD_P1002, > + MATRIX_BOARD_P1004, > + MATRIX_BOARD_P1008, > + MATRIX_BOARD_P1016, > + MATRIX_BOARD_P2002, > + MATRIX_BOARD_P2004, > + MATRIX_BOARD_P2008, > + MATRIX_BOARD_P3002, > + MATRIX_BOARD_P3004, > + MATRIX_BOARD_P3008, > + > +// sun1999-serial RS232 > + SUN1999_BOARD_5027A, > + SUN1999_BOARD_5037A, > + SUN1999_BOARD_5056A, > + SUN1999_BOARD_5066A, > + SUN1999_BOARD_5016A, > + > + //sun1999-multi I/O > + SUN1999_BOARD_5069A, > + SUN1999_BOARD_5079A, > + SUN1999_BOARD_5099A, > + > + //sun1999-parallel > + can we have a bit more consistent formatting (eg. all those comments w/ same indention) ? > + SUN1999_BOARD_5008A, > + <snip> > +static struct pci_device_id sunix_pci_board_id[] = { > +// golden-serial can't this be const ? (maybe even __initconst) > + {VENID_GOLDEN, DEVID_G_SERIAL, SUBVENID_GOLDEN, > + SUBDEVID_TEST, 0, 0, GOLDEN_BOARD_TEST}, > + > +// matrix-serial > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P1002, 0, 0, MATRIX_BOARD_P1002}, > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P1004, 0, 0, MATRIX_BOARD_P1004}, > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P1008, 0, 0, MATRIX_BOARD_P1008}, > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P1016, 0, 0, MATRIX_BOARD_P1016}, > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P2002, 0, 0, MATRIX_BOARD_P2002}, > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P2004, 0, 0, MATRIX_BOARD_P2004}, > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P2008, 0, 0, MATRIX_BOARD_P2008}, > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P3002, 0, 0, MATRIX_BOARD_P3002}, > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P3004, 0, 0, MATRIX_BOARD_P3004}, > + {VENID_MATRIX, DEVID_M_SERIAL, SUBVENID_MATRIX, > + SUBDEVID_P3008, 0, 0, MATRIX_BOARD_P3008}, > + > + // sun1999-serial RS232 > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_5027A, 0, 0, SUN1999_BOARD_5027A}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_5037A, 0, 0, SUN1999_BOARD_5037A}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_5056A, 0, 0, SUN1999_BOARD_5056A}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_5066A, 0, 0, SUN1999_BOARD_5066A}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_5016A, 0, 0, SUN1999_BOARD_5016A}, > + > + // sun1999-multi I/O > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_5069A, 0, 0, SUN1999_BOARD_5069A}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_5079A, 0, 0, SUN1999_BOARD_5079A}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_5099A, 0, 0, SUN1999_BOARD_5099A}, > + > + // sun1999-parallel > + {VENID_SUN1999, DEVID_S_PARALL, SUBVENID_SUN1999, > + SUBDEVID_5008A, 0, 0, SUN1999_BOARD_5008A}, > + > + // sun1999-serial RS422/485 > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_P2102, 0, 0, SUN1999_BOARD_P2102}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_P2104, 0, 0, SUN1999_BOARD_P2104}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_P2108, 0, 0, SUN1999_BOARD_P2108}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_P2116, 0, 0, SUN1999_BOARD_P2116}, > + > + // sun1999 3_in_1 > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_P3104, 0, 0, SUN1999_BOARD_P3104}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_P3108, 0, 0, SUN1999_BOARD_P3108}, > + > + //cash drawer card 2S > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_CASH_2S, 0, 0, SUN1999_BOARD_CASH_2S}, > + > + //cash drawer card 4S > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_CASH_4S, 0, 0, SUN1999_BOARD_CASH_4S}, > + > + //DIO > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_DIO0802, 0, 0, SUN1999_BOARD_DIO0802}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_DIO1604, 0, 0, SUN1999_BOARD_DIO1604}, > + {VENID_SUN1999, DEVID_S_SERIAL, SUBVENID_SUN1999, > + SUBDEVID_DIO3204, 0, 0, SUN1999_BOARD_DIO3204}, > + > + {0} > +}; > +MODULE_DEVICE_TABLE(pci, sunix_pci_board_id); > + > +struct sunix_board sunix_board_table[SNX_BOARDS_MAX]; > +struct sunix_ser_port sunix_ser_table[SNX_SER_TOTAL_MAX + 1]; > +struct sunix_par_port sunix_par_table[SNX_PAR_TOTAL_MAX]; 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 ? 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) > +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 ? > +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 ? <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 ? <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 ? > +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 ? <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 ? <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 ? > +static int sunix_pci_board_probe(void) > +{ > + struct sunix_board *sb; > + struct pci_dev *pdev = NULL; > + struct pci_dev *pdev_array[4] = {NULL, NULL, NULL, NULL}; > + > + int sunix_pci_board_id_cnt; > + int tablecnt; > + int boardcnt; > + int i; > + unsigned short int sub_device_id; > + unsigned short int device_part_number; > + unsigned int bar3_base_add; > + > + int status; > + unsigned int bar3_Byte5; > + unsigned int bar3_Byte6; > + unsigned int bar3_Byte7; > + unsigned int oem_id; > + unsigned char uart_type; > + unsigned char gpio_type; > + unsigned char gpio_card_type; > + int gpio_ch_cnt; > + > + // clear and init some variable > + memset(sunix_board_table, 0, SNX_BOARDS_MAX * > + sizeof(struct sunix_board)); > + > + for (i = 0; i < SNX_BOARDS_MAX; i++) { > + sunix_board_table[i].board_enum = -1; > + sunix_board_table[i].board_number = -1; if it already is a global, why not statically initialized ? > + } > + > + sunix_pci_board_id_cnt = > + (sizeof(sunix_pci_board_id) / sizeof(sunix_pci_board_id[0])) - 1; > + > + // search matrix serial board > + pdev = NULL; > + tablecnt = 0; > + boardcnt = 0; > + sub_device_id = 0; > + status = 0; > + > + while (tablecnt < sunix_pci_board_id_cnt) { > + > + pdev = pci_get_device(VENID_MATRIX, DEVID_M_SERIAL, pdev); > + > + if (pdev == NULL) { > + tablecnt++; > + continue; > + } > + > + if ((tablecnt > 0) && ((pdev == pdev_array[0]) || > + (pdev == pdev_array[1]) || > + (pdev == pdev_array[2]) || > + (pdev == pdev_array[3]))) { > + continue; > + } > + > + pci_read_config_word(pdev, 0x2e, &sub_device_id); > + > + if (sub_device_id == 0) { > + status = -EIO; > + return status; > + } > + > + if (sub_device_id != sunix_pci_board_id[tablecnt].subdevice) > + continue; > + if (pdev == NULL) { > + pr_err("SNX Error: PCI device object is NULL !\n"); > + status = -EIO; > + return status; > + > + } else { > + > + status = pci_enable_device(pdev); > + > + if (status != 0) { > + pr_err("SNX Error: SUNIX Board Enable Fail > !\n\n"); > + status = -ENXIO; > + return status; > + } > + } > + > + boardcnt++; > + if (boardcnt > SNX_BOARDS_MAX) { > + pr_err("SNX Error: Support Four Boards In Maximum !\n"); > + status = -ENOSPC; > + return status; > + } > + > + sb = &sunix_board_table[boardcnt-1]; > + pdev_array[boardcnt-1] = pdev; > + sb->pdev = pdev; > + sb->bus_number = pdev->bus->number; > + sb->dev_number = PCI_SLOT(pdev->devfn); > + sb->board_enum = (int)sunix_pci_board_id[tablecnt].driver_data; > + sb->pb_info = snx_pci_board_conf[sb->board_enum]; > + sb->board_flag = sb->pb_info.board_flag; > + sb->board_number = boardcnt - 1; > + } Why not doing this in snx_pci_probe() on per-board basis, and let device registration be done by pci subsystem ? > + // 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 ? <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. <snip> > +static int sunix_get_pci_board_conf(void) > +{ > + struct sunix_board *sb = NULL; > + struct pci_dev *pdev = NULL; > + int status = 0; > + int i; > + int j; > + > + for (i = 0; i < SNX_BOARDS_MAX; i++) { > + sb = &sunix_board_table[i]; > + > + if (sb->board_enum > 0) { > + pdev = sb->pdev; > + sb->ports = sb->pb_info.num_serport + > + sb->pb_info.num_parport; > + sb->ser_port = sb->pb_info.num_serport; > + sb->par_port = sb->pb_info.num_parport; > + snx_ser_port_total_cnt = snx_ser_port_total_cnt + > + sb->ser_port; > + snx_par_port_total_cnt = snx_par_port_total_cnt + > + sb->par_port; > + > + if (snx_ser_port_total_cnt > SNX_SER_TOTAL_MAX) { > + pr_err("SNX Error: Too much serial port\n"); > + status = -EIO; > + return status; > + } why that limitation ? why does this have to be a global array at all ? > +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 ? <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> > +static int __init snx_init(void) > +{ > + int status = 0; > + > + pr_err("SNX Info : Loading SUNIX Multi-I/O Board Driver Module\n"); > + > + snx_ser_port_total_cnt = snx_par_port_total_cnt = 0; > + > + status = sunix_pci_board_probe(); > + if (status != 0) > + goto step1_fail; > + > + status = sunix_get_pci_board_conf(); > + if (status != 0) > + goto step1_fail; > + > + status = sunix_assign_resource(); > + if (status != 0) > + goto step1_fail; > + > + status = sunix_ser_port_table_init(); > + if (status != 0) > + goto step1_fail; > + > + status = sunix_par_port_table_init(); > + if (status != 0) > + goto step1_fail; > + > + status = sunix_register_irq(); > + if (status != 0) > + goto step1_fail; > + > + status = sunix_ser_register_driver(&sunix_ser_reg); > + if (status != 0) > + goto step2_fail; > + > + status = sunix_ser_register_ports(&sunix_ser_reg); > + if (status != 0) > + goto step3_fail; > + > + status = pci_register_driver(&snx_pci_driver); > + if (status != 0) > + goto step7_fail; > + > + if (snx_par_port_total_cnt > 0) { > + status = sunix_par_parport_init(); > + if (status != 0) > + goto step4_fail; > + > + status = sunix_par_ppdev_init(); > + if (status != 0) > + goto step5_fail; > + > + status = sunix_par_lp_init(); > + if (status != 0) > + goto step6_fail; > + } why not doing those initializations on per-device basis, in the corresponding probe() function ? > +step7_fail: > + > + pci_unregister_driver(&snx_pci_driver); > +step6_fail: > + > + sunix_par_ppdev_exit(); > + > + > +step5_fail: > + > + sunix_par_parport_exit(); > + > + > +step4_fail: > + > + sunix_ser_unregister_ports(&sunix_ser_reg); > + } > + > +step3_fail: > + > + sunix_ser_unregister_driver(&sunix_ser_reg); > + > + > +step2_fail: > + > + sunix_release_irq(); use devm_*() functions, which automatically releases resource when a device is deleted, so explicit release isn't needed anymore. > +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. > 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 ? > +#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 ? <snip> > +static _INLINE_ void snx_ser_handle_cts_change( > + struct snx_ser_port *, unsigned int); Where's this strange "_INLINE_" coming from ? <snip> > +//extern void snx_ser_change_speed( > + //struct snx_ser_state *state, struct SNXTERMIOS *old_termios); please no dead/commented-out code. > +//extern int sunix_ser_interrupt( > + //struct sunix_board *, struct sunix_ser_port *first_sp); same here > +//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 > +static unsigned char READ_INTERRUPT_VECTOR_BYTE(struct sunix_ser_port *sp) i'd recommend no caps in function names. > + if (var == 0x04) { > + vet4 = inb(local_vector + 0xb0); > + vet4 <<= 12; > + } > + > + data = (vet1 | vet2 | vet3 | vet4); > + > + return data; > + } > + return 0; > +} > + > + > + remove excess newlines <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 ? <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 ? <nsip> > + tasklet_kill(&info->tlet); what exactly is the tasklet needed for ? > + if (!capable(CAP_SYS_ADMIN)) { > + retval = -EPERM; why this explicit check for CAP_SYS_ADMIN ? > + if (change_irq || indention mismatch. > + change_port || why is userland allowed to change irq, port, etc, if that's probed from pci anyways ? > + (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) > + 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 ? > + 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. <snip> > + case SNX_UART_SET_TYPE: { what is this for ? <snip> > + case SNX_UART_SET_ACS: what is "ACS" ? > + { > + 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 ? <snip> > + if (tty->flags & (1 << TTY_IO_ERROR)) { > + ret = -EIO; > + goto out; > + } > + > + switch (cmd) { > + case TIOCMIWAIT: yet another idention mismatch. <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. <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. <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. <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 ? <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) ? <snip> > +extern void sunix_ser_unregister_ports(struct snx_ser_driver *drv) why are these 'extern' ?! --mtx -- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering i...@metux.net -- +49-151-27565287