On Sat, Aug 19, 2006 at 04:06:44PM +0400, Vitaly Wool wrote:
> please find the patch that adds PNX8xxx UART support with your latest
> comments taken into account inlined.
Okay, I'm now happy with this - thanks for addressing those points so far.
Consider the following two comments non-show stoppers, which can be fixed
up later. If you do want to submit another patch which these two addressed
that's also fine - I won't be applying any patches for at least a couple
of hours. If not, I'll apply this patch as is.
> +static irqreturn_t pnx8xxx_int(int irq, void *dev_id, struct pt_regs *regs)
> +{
> + struct pnx8xxx_port *sport = dev_id;
> + unsigned int status;
> +
> + spin_lock(&sport->port.lock);
> + /* Get the interrupts */
> + status = serial_in(sport, PNX8XXX_ISTAT) & serial_in(sport, PNX8XXX_IEN);
> +
> + /* RX Receiver Holding Register Overrun */
> + if (status & PNX8XXX_UART_INT_RXOVRN) {
> + serial_out(sport, PNX8XXX_ICLR, PNX8XXX_UART_INT_RXOVRN);
> + }
> +
> + /* RX Frame Error */
> + if (status & PNX8XXX_UART_INT_FRERR) {
> + serial_out(sport, PNX8XXX_ICLR, PNX8XXX_UART_INT_FRERR);
> + }
> +
> + /* Break signal received */
> + if (status & PNX8XXX_UART_INT_BREAK) {
> + sport->port.icount.brk++;
> + serial_out(sport, PNX8XXX_ICLR, PNX8XXX_UART_INT_BREAK);
> + uart_handle_break(&sport->port);
> + }
> +
> + /* RX Parity Error */
> + if (status & PNX8XXX_UART_INT_PARITY) {
> + serial_out(sport, PNX8XXX_ICLR, PNX8XXX_UART_INT_PARITY);
> + }
> +
> + /* Byte received */
> + if (status & PNX8XXX_UART_INT_RX) {
> + pnx8xxx_rx_chars(sport, regs);
> + serial_out(sport, PNX8XXX_ICLR, PNX8XXX_UART_INT_RX);
> + }
> +
> + /* TX holding register empty - transmit a byte */
> + if (status & PNX8XXX_UART_INT_TX) {
> + pnx8xxx_tx_chars(sport);
> + serial_out(sport, PNX8XXX_ICLR, PNX8XXX_UART_INT_TX);
> + }
> +
> + /* TX shift register and holding register empty */
> + if (status & PNX8XXX_UART_INT_EMPTY) {
> + serial_out(sport, PNX8XXX_ICLR, PNX8XXX_UART_INT_EMPTY);
> + }
> +
> + /* Receiver time out */
> + if (status & PNX8XXX_UART_INT_RCVTO) {
> + serial_out(sport, PNX8XXX_ICLR, PNX8XXX_UART_INT_RCVTO);
> + }
Would it be more efficient to write to ICLR once at the end of the
function, rather than once per status bit? Could you do this instead:
if (status & PNX8XXX_UART_INT_BREAK) {
sport->port.icount.brk++;
serial_out(sport, PNX8XXX_ICLR, PNX8XXX_UART_INT_BREAK);
uart_handle_break(&sport->port);
}
if (status & PNX8XXX_UART_INT_RX)
pnx8xxx_rx_chars(sport, regs);
if (status & PNX8XXX_UART_INT_TX)
pnx8xxx_tx_chars(sport);
serial_out(sport, PNX8XXX_ICLR, status);
?
> + spin_unlock(&sport->port.lock);
> + return IRQ_HANDLED;
> +}
...
> +static void pnx8xxx_shutdown(struct uart_port *port)
> +{
> + struct pnx8xxx_port *sport = (struct pnx8xxx_port *)port;
> +
> + /*
> + * Stop our timer.
> + */
> + del_timer_sync(&sport->timer);
> +
> + /*
> + * Disable all interrupts, port and break condition.
> + */
> + serial_out(sport, PNX8XXX_IEN, 0);
> + serial_out(sport, PNX8XXX_LCR,
> + serial_in(sport, PNX8XXX_LCR) & ~PNX8XXX_UART_LCR_TXBREAK);
> +
> + /*
> + * Reset the Tx and Rx FIFOS
> + */
> + serial_out(sport, PNX8XXX_LCR, serial_in(sport, PNX8XXX_LCR) |
> + PNX8XXX_UART_LCR_TX_RST |
> + PNX8XXX_UART_LCR_RX_RST);
Hmm, two read-modify-writes to the LCR in succession. Wouldn't:
lcr = serial_in(sport, PNX8XXX_LCR);
lcr &= ~PNX8XXX_UART_LCR_TXBREAK;
lcr |= PNX8XXX_UART_LCR_TX_RST | PNX8XXX_UART_LCR_RX_RST;
serial_out(sport, PNX8XXX_LCR, lcr);
be more efficient/smaller code?
--
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
