Messages in this thread | | | Subject | Re: [PATCH V6 1/1] usb:serial: Add Fintek F81532/534 driver | From | Peter Hung <> | Date | Wed, 4 Nov 2015 16:19:39 +0800 |
| |
Hi
Oliver Neukum 於 2015/11/3 下午 06:03 寫道: > On Tue, 2015-11-03 at 11:51 +0800, Peter Hung wrote: >> +static int f81534_attach(struct usb_serial *serial) >> +{ >> + struct f81534_serial_private *serial_priv = NULL; >> + int status; >> + int i; >> + int offset; >> + uintptr_t setting_idx = (uintptr_t) usb_get_serial_data(serial); >> + >> + serial_priv = kzalloc(sizeof(*serial_priv), GFP_KERNEL); >> + if (!serial_priv) >> + return -ENOMEM; >> + >> + usb_set_serial_data(serial, serial_priv); >> + serial_priv->setting_idx = setting_idx; >> + >> + for (i = 0; i < F81534_NUM_PORT; ++i) { >> + /* Disable all interrupt before submit URB */ >> + status = f81534_setregister(serial->dev, i, >> + INTERRUPT_ENABLE_REGISTER, 0x00); >> + if (status) { >> + dev_err(&serial->dev->dev, "%s: IER disable failed\n", >> + __func__); >> + goto failed; >> + } >> + } >> + >> + for (i = 0; i < F81534_NUM_PORT; ++i) >> + atomic_set(&serial_priv->port_active[i], 0); > > Should be ATOMIC_INIT() >
ATOMIC_INIT() seems to be used only for variable initializer, It cant be used for dynamic allocation. Should I change it to a normal boolean flag protecting with spin_lock ?
>> +static int f81534_port_remove(struct usb_serial_port *port) >> +{ >> + struct f81534_port_private *port_priv; >> + >> + f81534_release_gpio(port); >> + port_priv = usb_get_serial_port_data(port); >> + kfree(port_priv); >> + return 0; >> +} >> + >> +static void f81534_compare_msr(struct usb_serial_port *port, u8 *msr, > > Is the point of passing a pointer to msr locking? > >> + bool is_port_open)
This function is used only with URB callback function. The *msr is reported by H/W with newest MSR. The USB-Serial generic system will re-submit read URB when callback complete. So this function should run once on the same time.
>> +static int f81534_tiocmget(struct tty_struct *tty) >> +{ >> + struct usb_serial_port *port = tty->driver_data; >> + struct f81534_port_private *port_priv = usb_get_serial_port_data(port); >> + unsigned long flags; >> + int r; >> + u8 msr, mcr; >> + >> + /* >> + * We'll avoid to direct read MSR register. The IC will read the MSR >> + * changed and report it f81534_process_per_serial_block() by >> + * F81534_TOKEN_MSR_CHANGE. >> + * >> + * When this device in heavy loading (e.g., BurnInTest Loopback Test) >> + * The report of MSR register will delay received a bit. It's due to >> + * MSR interrupt is lowest priority in 16550A. So we decide to sleep >> + * a little time to pass the test. >> + */ >> + if (schedule_timeout_interruptible( >> + msecs_to_jiffies(F81534_DELAY_READ_MSR))) { >> + dev_info(&port->dev, "%s: breaked !!\n", __func__); >> + } > > Is the delay necessary or isn't it? > If it is necessary you should do something about the signal. >
We add this delay due to stress test (Loop-back & 921600bps with BurnInTest). It'll receive MSR with some delay when connecting with DTR-DSR & RTS/CTS, but the delay smaller than 10ms. So we decided to delay some time to pass the test.
>> +static int f81534_prepare_write_buffer(struct usb_serial_port *port, >> + void *dest, size_t size) >> +{ >> + unsigned char *ptr = (unsigned char *) dest; >> + struct f81534_port_private *port_priv = usb_get_serial_port_data(port); >> + int port_num = port_priv->phy; >> + struct usb_serial *serial = port->serial; >> + >> + WARN_ON(size != serial->port[0]->bulk_out_size); >> + >> + if (size != F81534_WRITE_BUFFER_SIZE) { >> + WARN_ON(size != F81534_WRITE_BUFFER_SIZE); > > What is the sense of this? >
I'll remove the double-check section with next version patch.
>> + ptr[F81534_RECEIVE_BLOCK_SIZE * 0] = 0; >> + ptr[F81534_RECEIVE_BLOCK_SIZE * 1] = 1; >> + ptr[F81534_RECEIVE_BLOCK_SIZE * 2] = 2; >> + ptr[F81534_RECEIVE_BLOCK_SIZE * 3] = 3; > > Either these ... > >> + ptr[F81534_RECEIVE_BLOCK_SIZE * port_num + 0] = port_num; > > .. or that is redundant >
I'll remove it too.
Thanks for your advice. -- With Best Regards, Peter Hung
| |