From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

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@bp.renesas.com>
---
 drivers/tty/serial/sh-sci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 1c356544a832..24773e265fbe 100644
--- 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);
 	}
 }
 
@@ -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);
 	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);
 	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);
 	}
 
 	/*
@@ -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);
 }
 
 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);
 	}
 }
 
-- 
2.49.0

Hi Prabhakar,

On Mon, 16 Jun 2025 at 23:39, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> 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@bp.renesas.com>

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@linux-m68k.org

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

Hi Geert,

Thank you for the review.

On Tue, Jun 17, 2025 at 9:44 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, 16 Jun 2025 at 23:39, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > 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@bp.renesas.com>
>
> 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