Messages in this thread | | | Subject | Re: [PATCH 2/4] Add SUNIX Multi-I/O card device driver | From | "Enrico Weigelt, metux IT consult" <> | Date | Fri, 8 Mar 2019 21:19:27 +0100 |
| |
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 info@metux.net -- +49-151-27565287
| |