Re: [PATCH v11 3/5] tty: serial: sh-sci: Use port ops callbacks

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Tue, Jun 17, 2025 at 9:44 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Mon, 16 Jun 2025 at 23:39, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Replace direct calls to internal helpers such as sci_stop_tx(),
> > sci_start_tx(), sci_stop_rx(), sci_set_mctrl(), sci_enable_ms(), and
> > sci_request_port() with their corresponding port ops callbacks.
> >
> > This change improves consistency and abstraction across the driver and
> > prepares the codebase for adding support for the RSCI driver on the
> > Renesas RZ/T2H SoC, which heavily reuses the existing SCI driver.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> I am a bit reluctant to increase the number of indirect calls in a
> driver that is also used on (old and slow) SH systems...
>
Ok, I will do that. My initial thought was just to replace the direct
calls where it's shared, But for consistency I replaced them all.

> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -880,7 +880,7 @@ static void sci_transmit_chars(struct uart_port *port)
> >                         sci_serial_out(port, SCSCR, ctrl);
> >                 }
> >
> > -               sci_stop_tx(port);
> > +               s->port.ops->stop_tx(port);
>
> RSCI has its own implementation of sci_port_ops.transmit_chars(), so
> I think it is better to avoid the overhead of an indirect call, and keep
> calling sci_stop_tx() directly.
>
Ok, I will drop this change.

>          }
> >  }
> >
> > @@ -1497,7 +1497,7 @@ static void sci_dma_tx_work_fn(struct work_struct *work)
> >  switch_to_pio:
> >         uart_port_lock_irqsave(port, &flags);
> >         s->chan_tx = NULL;
> > -       sci_start_tx(port);
> > +       s->port.ops->start_tx(port);
>
> This function is indeed shared by sh-sci and rsci, but still unused
> by the latter as it does not support DMA yet.
>
Ok, I will drop this change.

> >         uart_port_unlock_irqrestore(port, flags);
> >         return;
> >  }
> > @@ -2289,8 +2289,8 @@ void sci_shutdown(struct uart_port *port)
> >         mctrl_gpio_disable_ms_sync(to_sci_port(port)->gpios);
> >
> >         uart_port_lock_irqsave(port, &flags);
> > -       sci_stop_rx(port);
> > -       sci_stop_tx(port);
> > +       s->port.ops->stop_rx(port);
> > +       s->port.ops->stop_tx(port);
>
> OK.
>
> >         s->ops->shutdown_complete(port);
> >         uart_port_unlock_irqrestore(port, flags);
> >
> > @@ -2684,7 +2684,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
> >         }
> >         if (port->flags & UPF_HARD_FLOW) {
> >                 /* Refresh (Auto) RTS */
> > -               sci_set_mctrl(port, port->mctrl);
> > +               s->port.ops->set_mctrl(port, port->mctrl);
>
> RSCI has its own implementation of uart_ops.set_termios(), so please
> keep the direct call.
>
Ok, I will drop this change.

> >         }
> >
> >         /*
> > @@ -2721,7 +2721,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
> >         sci_port_disable(s);
> >
> >         if (UART_ENABLE_MS(port, termios->c_cflag))
> > -               sci_enable_ms(port);
> > +               s->port.ops->enable_ms(port);
>
> Likewise.
> And once RSCI fully implements uart_ops.set_termios(), I think
> it can just reuse sci_enable_ms().
>
Ok, I will drop this change.

> >  }
> >
> >  void sci_pm(struct uart_port *port, unsigned int state,
> > @@ -2827,7 +2827,7 @@ void sci_config_port(struct uart_port *port, int flags)
> >                 struct sci_port *sport = to_sci_port(port);
> >
> >                 port->type = sport->cfg->type;
> > -               sci_request_port(port);
> > +               sport->port.ops->request_port(port);
>
> Both sh-sci and rsci use sci_request_port() as their
> uart_ops.request_port() callbacks, so please use a direct call.
>
Ok, I will drop this change.

Cheers,
Prabhakar