On Wed, 2015-11-04 at 16:19 +0800, Peter Hung wrote:
> 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 ?
No, if it doesn't work, use the current code.
> >> +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.
Yes, so why don't you pass an u8 as opposed to a pointer to an u8?
> >> +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.
OK, but how do you guarantee the delay you need if you get a signal,
which would abort the delay?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
