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" <[email protected]>
Sent: Saturday, February 19, 2011 12:21 AM
To: <[email protected]>
Cc: "Thomas Gleixner" <[email protected]>; "Alan Cox"
<[email protected]>; <[email protected]>;
<[email protected]>; "Subhasish Ghosh"
<[email protected]>; "Greg Kroah-Hartman" <[email protected]>;
<[email protected]>; "open list" <[email protected]>;
<[email protected]>
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" <[email protected]> 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
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
