lkml.org 
[lkml]   [2015]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V6 1/1] usb:serial: Add Fintek F81532/534 driver
From
Date
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


\
 
 \ /
  Last update: 2015-11-04 09:41    [W:0.111 / U:1.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site