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...
> --- 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.
}
> }
>
> @@ -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.
> 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.
> }
>
> /*
> @@ -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().
> }
>
> 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.
> }
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
