On Thu, Nov 10, 2022 at 01:01:47PM +0200, Ilpo Järvinen wrote:
> On Wed, 9 Nov 2022, Gabriel Somlo wrote:
>
> > Move the character-receive (RX) loop to its own dedicated function,
> > and (for now) call that from the poll timer, liteuart_timer().
> >
> > This is in preparation for adding IRQ support to the receive path.
> >
> > Signed-off-by: Gabriel Somlo <gsomlo@xxxxxxxxx>
> > ---
> > drivers/tty/serial/liteuart.c | 25 +++++++++++++++----------
> > 1 file changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> > index 047d5ad32e13..aa7052280197 100644
> > --- a/drivers/tty/serial/liteuart.c
> > +++ b/drivers/tty/serial/liteuart.c
> > @@ -67,29 +67,34 @@ static struct uart_driver liteuart_driver = {
> > #endif
> > };
> >
> > -static void liteuart_timer(struct timer_list *t)
> > +static void liteuart_rx_chars(struct uart_port *port)
> > {
> > - struct liteuart_port *uart = from_timer(uart, t, timer);
> > - struct uart_port *port = &uart->port;
> > unsigned char __iomem *membase = port->membase;
> > - unsigned int flg = TTY_NORMAL;
> > - int ch;
> > - unsigned long status;
> > + unsigned int status;
> > + unsigned char ch;
> >
> > while ((status = !litex_read8(membase + OFF_RXEMPTY)) == 1) {
> > ch = litex_read8(membase + OFF_RXTX);
> > port->icount.rx++;
> >
> > /* necessary for RXEMPTY to refresh its value */
> > - litex_write8(membase + OFF_EV_PENDING, EV_TX | EV_RX);
> > + litex_write8(membase + OFF_EV_PENDING, EV_RX);
> >
> > /* no overflow bits in status */
> > if (!(uart_handle_sysrq_char(port, ch)))
> > - uart_insert_char(port, status, 0, ch, flg);
> > -
> > - tty_flip_buffer_push(&port->state->port);
> > + uart_insert_char(port, status, 0, ch, TTY_NORMAL);
> > }
> >
> > + tty_flip_buffer_push(&port->state->port);
>
> This change is doing extra stuff besides moving rx to a dedicated
> function.
>
> I see no reason why those other changes couldn't be put into an entirely
> separate patch. Also, please described those changes properly in the
> commit message (answer the why? question).
You're right, calling `tty_flip_buffer_push()` as each character is
received is overkill, we only need to call it once per interrupt once
all available characters have been received.
I forgot I noticed (and fixed) that as part of the move -- I'll split
it out into its own separate patch (probably *before* moving all of rx
to a dedicated function).
Should show up in v3, once I also address all the other feedback I
received.
Thanks again for catching it!
Cheers,
-Gabriel
> --
> i.
>
> > +}
> > +
> > +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);
> > +
> > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
> > }
> >
> >
>
>
