On Tue, Nov 15, 2022 at 06:21:00PM +0200, Ilpo Järvinen wrote: > On Tue, 15 Nov 2022, Gabriel L. Somlo wrote: > > > On Tue, Nov 15, 2022 at 06:00:11PM +0200, Ilpo Järvinen wrote: > > > On Sat, 12 Nov 2022, Gabriel Somlo wrote: > > > > > > > Add support for IRQ-driven RX. Support for the TX path will be added > > > > in a separate commit. > > > > > > > > Signed-off-by: Gabriel Somlo <gsomlo@xxxxxxxxx> > > > > --- > > > > drivers/tty/serial/liteuart.c | 61 +++++++++++++++++++++++++++++++---- > > > > 1 file changed, 54 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c > > > > index cf1ce597b45e..e30adb30277f 100644 > > > > --- a/drivers/tty/serial/liteuart.c > > > > +++ b/drivers/tty/serial/liteuart.c > > > > @@ -6,6 +6,7 @@ > > > > */ > > > > > > > > #include <linux/console.h> > > > > +#include <linux/interrupt.h> > > > > #include <linux/litex.h> > > > > #include <linux/module.h> > > > > #include <linux/of.h> > > > > @@ -130,13 +131,29 @@ static void liteuart_rx_chars(struct uart_port *port) > > > > tty_flip_buffer_push(&port->state->port); > > > > } > > > > > > > > +static irqreturn_t liteuart_interrupt(int irq, void *data) > > > > +{ > > > > + struct liteuart_port *uart = data; > > > > + struct uart_port *port = &uart->port; > > > > + u8 isr = litex_read8(port->membase + OFF_EV_PENDING); > > > > + > > > > + /* for now, only rx path triggers interrupts */ > > > > > > Please don't add comment like this at all when your series removes it in a > > > later patch. > > > > OK, I will remove it in v4. > > > > > > + isr &= EV_RX; > > > > + > > > > + spin_lock(&port->lock); > > > > + if (isr & EV_RX) > > > > + liteuart_rx_chars(port); > > > > + spin_unlock(&port->lock); > > > > + > > > > + return IRQ_RETVAL(isr); > > > > +} > > > > + > > > > static void liteuart_timer(struct timer_list *t) > > > > { > > > > struct liteuart_port *uart = from_timer(uart, t, timer); > > > > struct uart_port *port = &uart->port; > > > > > > > > - liteuart_rx_chars(port); > > > > - > > > > + liteuart_interrupt(0, port); > > > > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port)); > > > > } > > > > > > > > @@ -162,19 +179,42 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port) > > > > static int liteuart_startup(struct uart_port *port) > > > > { > > > > struct liteuart_port *uart = to_liteuart_port(port); > > > > + int ret; > > > > + u8 irq_mask = 0; > > > > > > > > - /* disable events */ > > > > - litex_write8(port->membase + OFF_EV_ENABLE, 0); > > > > + if (port->irq) { > > > > + ret = request_irq(port->irq, liteuart_interrupt, 0, > > > > + KBUILD_MODNAME, uart); > > > > + if (ret == 0) { > > > > + /* only enable rx interrupts at this time */ > > > > > > This comment seems pretty useless. Your code says very much the same. > > > > The comment was meant to let the reader know that the code is doing it > > *intentionally* (rather than forgetting to enable tx irqs by mistake). > > But I'm OK with removing this comment in v4 as well if you think > > that's an overly paranoid and redundant thing to do... :) > > I see. Reading the other comment first caused me to misinterpret this one > to mean that only RX interrupts are implemented. > > Maybe if you change "at this time" to "at startup" it would make it more > obvious. OK, I'll fix the comment per your suggestion rather than get rid of it. Thanks again, --G > -- > i.