Re: [PATCH v4 5/5] tty: serial: sh-sci: Fix end of transmission on SCI

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

On Fri, 21 Apr 2023, Geert Uytterhoeven wrote:

> Hi Biju,
> 
> On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > We need to set TE to "0" (i.e., disable serial transmission) to
> > get the expected behavior of the end of serial transmission.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -847,6 +847,11 @@ static void sci_transmit_chars(struct uart_port *port)
> >                 } else if (!uart_circ_empty(xmit) && !stopped) {
> >                         c = xmit->buf[xmit->tail];
> >                         xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> > +               } else if (port->type == PORT_SCI && uart_circ_empty(xmit)) {
> > +                       ctrl = serial_port_in(port, SCSCR);
> > +                       ctrl &= ~SCSCR_TE;
> > +                       serial_port_out(port, SCSCR, ctrl);
> > +                       return;
> 
> So nothing after the while loop should be done anymore?
> What about clearing SCSCR_TE in sci_stop_tx() (which is called after
> the while loop) instead?

Yes, this added fragment doesn't really seem to belong into the tx loop.

The right direction would be to aim towards converting the whole tx loop 
into using uart_port_tx_limited().

-- 
 i.

> >                 } else {
> >                         break;
> >                 }
> 
> So combined with my comments on patch 4/5, that would become
> 
> -    if (port->type == PORT_SCI)
> +    if (port->type == PORT_SCI) {
>              ctrl |= SCSCR_TEIE;
> +            ctrl &= ~SCSCR_TE;
> +    }
> 
> in sci_stop_tx().