Messages in this thread | | | Date | Mon, 13 Dec 2021 08:47:11 +0100 | Subject | Re: [PATCH v5 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver | From | Jiri Slaby <> |
| |
On 13. 12. 21, 8:10, Hammer Hsieh wrote: > Add Sunplus SoC UART Driver > > Signed-off-by: Hammer Hsieh <hammer.hsieh@sunplus.com>
...
> --- /dev/null > +++ b/drivers/tty/serial/sunplus-uart.c
...
> +static void receive_chars(struct uart_port *port) > +{ > + unsigned int lsr = readl(port->membase + SUP_UART_LSR); > + unsigned int ch, flag; > + > + do { > + ch = readl(port->membase + SUP_UART_DATA); > + flag = TTY_NORMAL; > + port->icount.rx++; > + > + if (unlikely(lsr & SUP_UART_LSR_BRK_ERROR_BITS)) { > + if (lsr & SUP_UART_LSR_BC) { > + lsr &= ~(SUP_UART_LSR_FE | SUP_UART_LSR_PE); > + port->icount.brk++; > + if (uart_handle_break(port)) > + goto ignore_char; > + } else if (lsr & SUP_UART_LSR_PE) { > + port->icount.parity++; > + } else if (lsr & SUP_UART_LSR_FE) { > + port->icount.frame++; > + } > + > + if (lsr & SUP_UART_LSR_OE) > + port->icount.overrun++; > + > + if (lsr & SUP_UART_LSR_BC) > + flag = TTY_BREAK; > + else if (lsr & SUP_UART_LSR_PE) > + flag = TTY_PARITY; > + else if (lsr & SUP_UART_LSR_FE) > + flag = TTY_FRAME;
Why do you handle these separately and not above?
> + } > + > + if (port->ignore_status_mask & SUP_DUMMY_READ) > + goto ignore_char; > + > + if (uart_handle_sysrq_char(port, ch)) > + goto ignore_char; > + > + uart_insert_char(port, lsr, SUP_UART_LSR_OE, ch, flag); > + > +ignore_char: > + lsr = readl(port->membase + SUP_UART_LSR); > + } while (lsr & SUP_UART_LSR_RX); > + > + tty_flip_buffer_push(&port->state->port); > +} > + > +static irqreturn_t sunplus_uart_irq(int irq, void *args) > +{ > + struct uart_port *port = (struct uart_port *)args;
No need to cast here.
> + unsigned int isc = readl(port->membase + SUP_UART_ISC);
Shouldn't this be under the spinlock?
And "if (!isc) return IRQ_NONE"?
> + spin_lock(&port->lock); > + > + if (isc & SUP_UART_ISC_RX) > + receive_chars(port); > + > + if (isc & SUP_UART_ISC_TX) > + transmit_chars(port); > + > + spin_unlock(&port->lock); > + > + return IRQ_HANDLED; > +} > + > +static int sunplus_startup(struct uart_port *port) > +{ > + unsigned long flags; > + unsigned int isc; > + int ret; > + > + ret = request_irq(port->irq, sunplus_uart_irq, 0, "sunplus_uart", port);
Cannot the interrupt be shared?
> + if (ret) > + return ret; > + > + spin_lock_irqsave(&port->lock, flags); > + > + isc |= SUP_UART_ISC_RXM; > + writel(isc, port->membase + SUP_UART_ISC); > + > + spin_unlock_irqrestore(&port->lock, flags); > + > + return 0; > +} > + > +static void sunplus_shutdown(struct uart_port *port) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&port->lock, flags); > + writel(0, port->membase + SUP_UART_ISC);
What bus is this -- posting?
> + spin_unlock_irqrestore(&port->lock, flags); > + > + free_irq(port->irq, port); > +}
...
> +static void sunplus_release_port(struct uart_port *port) > +{ > +} > + > +static int sunplus_request_port(struct uart_port *port) > +{ > + return 0; > +}
These two are optional -- no need to provide them.
regards, -- js suse labs
| |