lkml.org 
[lkml]   [2005]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2.6] Altix - ioc3 serial support
    On Fri, Dec 16, 2005 at 04:33:26PM -0600, Pat Gefre wrote:
    > The following patch adds driver support for a 2 port PCI IOC3-based
    > serial card on Altix boxes:
    >
    > ftp://oss.sgi.com/projects/sn2/sn2-update/044-ioc3-support

    Here's some comments on ioc3_serial.c. Could you look at them and
    either resolve them or discuss further. Thanks.

    +#include <linux/serialP.h>

    I don't think you need this include.

    + the_port->timeout = ((the_port->fifosize * HZ * bits) / (baud / 10));
    + the_port->timeout += HZ / 50; /* Add .02 seconds of slop */

    Please use uart_update_timeout() instead.

    + info = the_port->info;
    + if (info->tty) {
    + set_bit(TTY_IO_ERROR, &info->tty->flags);
    + clear_bit(TTY_IO_ERROR, &info->tty->flags);
    + if ((info->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
    + info->tty->alt_speed = 57600;
    + if ((info->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
    + info->tty->alt_speed = 115200;
    + if ((info->flags & ASYNC_SPD_MASK) == ASYNC_SPD_SHI)
    + info->tty->alt_speed = 230400;
    + if ((info->flags & ASYNC_SPD_MASK) == ASYNC_SPD_WARP)
    + info->tty->alt_speed = 460800;
    + }

    None of this is required. info->tty->alt_speed is not used by the
    serial layer - it knows how to deal with this itself. Secondly,
    setting and clearing TTY_IO_ERROR is pointless. Note that the serial
    layer takes care of TTY_IO_ERROR handling for you.

    + /* set the speed of the serial port */
    + ioc3_change_speed(the_port, info->tty->termios, (struct termios *)0);

    serial_core will call this for you at the appropriate time. Note that
    you decided above to check whether info->tty was NULL. If it was this
    will oops. Better just get rid of it anyway - it's not necessary.

    + /* Notify upper layer of carrier drop */
    + if ((port->ip_notify & N_DDCD)
    + && port->ip_port) {
    + the_port->icount.dcd = 0;
    + wake_up_interruptible
    + (&the_port->info->
    + delta_msr_wait);
    + }

    Use uart_handle_dcd_change(). Setting port->icount.dcd to zero in
    this case is wrong. It also makes no attempt at informing the upper
    layers that a hangup occurred. Note that uart_handle_dcd_change()
    exists so that you don't have to think about these semantics. You
    will need to keep the wake_up_interruptible though.

    + if ((port->ip_notify & N_DDCD)
    + && (shadow & SHADOW_DCD)
    + && (port->ip_port)) {
    + the_port = port->ip_port;
    + the_port->icount.dcd = 1;
    + wake_up_interruptible
    + (&the_port->info->delta_msr_wait);

    Ditto. icount.dcd is not the state of DCD. It is a counter for the
    number of times DCD changes state.

    + if ((port->ip_notify & N_DCTS) && (port->ip_port)) {
    + the_port = port->ip_port;
    + the_port->icount.cts =
    + (shadow & SHADOW_CTS) ? 1 : 0;
    + wake_up_interruptible
    + (&the_port->info->delta_msr_wait);
    + }

    Ditto, except uart_handle_cts_change().

    + the_port->lock = SPIN_LOCK_UNLOCKED;
    + spin_lock_init(&the_port->lock);

    Not necessary - uart_add_one_port() does this for you for non-console
    ports, and for console ports, it is assumed that the console code has
    already initialised the spinlock.

    + if (request_count > TTY_FLIPBUF_SIZE - tty->flip.count)
    + request_count = TTY_FLIPBUF_SIZE - tty->flip.count;
    +
    + if (request_count > 0) {
    + read_count = do_read(the_port, ch, request_count);
    + if (read_count > 0) {
    + flip = 1;
    + memcpy(tty->flip.char_buf_ptr, ch, read_count);
    + memset(tty->flip.flag_buf_ptr, TTY_NORMAL, read_count);
    + tty->flip.char_buf_ptr += read_count;
    + tty->flip.flag_buf_ptr += read_count;
    + tty->flip.count += read_count;
    + the_port->icount.rx += read_count;
    + }
    + }

    Please talk to Alan Cox about the best way to handle this. flip
    buffers are going away.

    +/**
    + * ic3_tx_empty - Is the transmitter empty? We pretend we're always empty
    + * @port: Port to operate on (we ignore since we always return 1)
    + *
    + */
    +static unsigned int ic3_tx_empty(struct uart_port *the_port)
    +{
    + return TIOCSER_TEMT;
    +}

    Not really a good idea if you care about the last bytes of data
    in various buffers. Eg, cat file > /dev/yourport could well chop
    off the last few characters for transmission.

    Finally, you register the uart driver in ioc3uart_init(), and
    unregister it in ioc3uart_remove() rather than ioc3uart_exit().
    What if you have multiple boards? You remove one of them and
    the uart driver gets unregistered? It doesn't look sane.

    Haven't looked at the rest of the code tho.

    --
    Russell King
    Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
    maintainer of: 2.6 Serial core
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-12-17 00:45    [W:0.029 / U:29.204 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site