lkml.org 
[lkml]   [2011]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH v2 13/13] tty: pruss SUART driver
    Date
    I agree with the Codingstyle which needs to be fixed and would appreciate 
    more feedback on it.

    I have also gotten rid of the semaphore completely, please let me know what
    you feel of this implementation:
    I have tested this without any problem.
    What I am basically doing below is that, I am getting the data from the circ
    buff and then using the interrupt handler to
    pump out the data. As the circ buff empties, I am accepting another request
    from the TTY.
    Are you noticing any problems with this:

    diff --git a/drivers/tty/serial/da8xx_pruss/pruss_suart.c
    b/drivers/tty/serial/da8xx_pruss/pruss_suart.c
    index d222e2e..edc3863 100644
    --- a/drivers/tty/serial/da8xx_pruss/pruss_suart.c
    +++ b/drivers/tty/serial/da8xx_pruss/pruss_suart.c
    @@ -72,7 +72,7 @@ struct suart_fifo {
    struct omapl_pru_suart {
    struct uart_port port[NR_SUART];
    struct device *dev; /* pdev->dev */
    - struct semaphore port_sem[NR_SUART];
    + bool tx_empty[NR_SUART];
    struct clk *clk_mcasp;
    struct suart_fifo suart_fifo_addr[NR_SUART];
    const struct firmware *fw;
    @@ -122,13 +122,10 @@ static void omapl_pru_tx_chars(struct omapl_pru_suart
    *soft_uart, u32 uart_no)
    if (!(suart_get_duplex(soft_uart, uart_no) & ePRU_SUART_HALF_TX))
    return;

    - if (down_trylock(&soft_uart->port_sem[uart_no]))
    - return;
    -
    if (uart_circ_empty(xmit) ||
    uart_tx_stopped(&soft_uart->port[uart_no])) {
    pruss_suart_stop_tx(&soft_uart->port[uart_no]);
    - up(&soft_uart->port_sem[uart_no]);
    + soft_uart->tx_empty[uart_no] = true;
    return;
    }

    @@ -259,7 +256,6 @@ static irqreturn_t pruss_suart_interrupt(s32 irq, void
    *dev_id)
    pru_intr_clr_isrstatus(dev, uart_num, PRU_TX_INTR);
    pru_softuart_clr_tx_status(dev,
    &soft_uart->suart_hdl
    [port->line]);
    - up(&soft_uart->port_sem[port->line]);
    omapl_pru_tx_chars(soft_uart, port->line);
    }
    } while (txrx_flag & (PRU_RX_INTR | PRU_TX_INTR));
    @@ -294,7 +290,10 @@ static void pruss_suart_start_tx(struct uart_port
    *port)

    suart_intr_setmask(dev, soft_uart->suart_hdl[port->line].uart_num,
    PRU_TX_INTR, CHN_TXRX_IE_MASK_CMPLT);
    - omapl_pru_tx_chars(soft_uart, port->line);
    + if (soft_uart->tx_empty[port->line] == true) {
    + soft_uart->tx_empty[port->line] = false;
    + omapl_pru_tx_chars(soft_uart, port->line);
    + }
    }


    --------------------------------------------------
    From: "Arnd Bergmann" <arnd@arndb.de>
    Sent: Saturday, February 19, 2011 12:21 AM
    To: <linux-arm-kernel@lists.infradead.org>
    Cc: "Thomas Gleixner" <tglx@linutronix.de>; "Alan Cox"
    <alan@lxorguk.ukuu.org.uk>; <sachi@mistralsolutions.com>;
    <davinci-linux-open-source@linux.davincidsp.com>; "Subhasish Ghosh"
    <subhasish@mistralsolutions.com>; "Greg Kroah-Hartman" <gregkh@suse.de>;
    <nsekhar@ti.com>; "open list" <linux-kernel@vger.kernel.org>;
    <m-watkins@ti.com>
    Subject: Re: [PATCH v2 13/13] tty: pruss SUART driver
    > On Friday 18 February 2011 19:23:49 Thomas Gleixner wrote:
    >> On Fri, 18 Feb 2011, Alan Cox wrote:
    >>
    >> > On Fri, 18 Feb 2011 19:17:38 +0530
    >> > "Subhasish Ghosh" <subhasish@mistralsolutions.com> wrote:
    >> >
    >> > > Hello,
    >> > >
    >> > > Regarding the semaphore to mutex migration.
    >> > > We are using down_trylock in interrupt context,
    >> > > mutex_trylock cannot be used in interrupt context, so we cannot use
    >> > > mutex in
    >> > > our driver.
    >> >
    >> > Then you probably need to rework your locking. Best bet might be to fix
    >> > all the other stuff and report the driver, and people can think about
    >> > the
    >> > locking problem.
    >>
    >> That semaphore is utterly useless to begin with. There are more
    >> serious locking problems than this one. Non serialized calls to
    >> suart_intr_clrmask/suart_intr_setmask are the most obvious ones.
    >>
    >> Aside of that the code is complete unreadable.
    >
    > I think it mostly suffers from the same problem as the CAN driver
    > I commented on earlier: One of the files (pruss_suart_api.c) was
    > clearly not written with Linux as the target, and the other files
    > try to work around this by wrapping a Linux driver around it.
    >
    > The suart_api HAL stuff clearly needs to go away, so that the rest
    > can be rewritten into a proper device driver.
    >
    > Arnd



    \
     
     \ /
      Last update: 2011-02-22 09:45    [W:0.033 / U:93.448 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site