Messages in this thread | | | Date | Sat, 19 Aug 2006 16:58:37 +0100 | From | Russell King <> | Subject | Re: [PATCH/RFC] UART driver for PNX8330/8550/8950: next iteration |
| |
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 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/
| |