On Thu, 1 Sep 2022, Jiri Slaby wrote:
> DEFINE_UART_PORT_TX_HELPER() is a new helper to send characters to the
> device. Use it in these drivers.
>
> Cc: Tobias Klauser <tklauser@xxxxxxxxxx>
> Cc: Richard Genoud <richard.genoud@xxxxxxxxx>
> Cc: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
> Cc: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> Cc: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
> Cc: Vladimir Zapolskiy <vz@xxxxxxxxx>
> Cc: Liviu Dudau <liviu.dudau@xxxxxxx>
> Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Cc: Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>
> Cc: Fabio Estevam <festevam@xxxxxxxxx>
> Cc: NXP Linux Team <linux-imx@xxxxxxx>
> Cc: "Andreas Färber" <afaerber@xxxxxxx>
> Cc: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> Signed-off-by: Jiri Slaby <jslaby@xxxxxxx>
> ---
> diff --git a/drivers/tty/serial/mpc52xx_uart.c b/drivers/tty/serial/mpc52xx_uart.c
> index 3f1986c89694..944686a0d00d 100644
> --- a/drivers/tty/serial/mpc52xx_uart.c
> +++ b/drivers/tty/serial/mpc52xx_uart.c
> @@ -1338,7 +1338,6 @@ mpc52xx_uart_verify_port(struct uart_port *port, struct serial_struct *ser)
> return 0;
> }
>
> -
> static const struct uart_ops mpc52xx_uart_ops = {
> .tx_empty = mpc52xx_uart_tx_empty,
> .set_mctrl = mpc52xx_uart_set_mctrl,
Stray change.
> diff --git a/drivers/tty/serial/sa1100.c b/drivers/tty/serial/sa1100.c
> index e64e42a19d1a..738f4b20cb8e 100644
> --- a/drivers/tty/serial/sa1100.c
> +++ b/drivers/tty/serial/sa1100.c
> @@ -226,45 +226,34 @@ sa1100_rx_chars(struct sa1100_port *sport)
> tty_flip_buffer_push(&sport->port.state->port);
> }
>
> -static void sa1100_tx_chars(struct sa1100_port *sport)
> +static bool sa1100_tx_ready(struct uart_port *port)
> {
> - struct circ_buf *xmit = &sport->port.state->xmit;
> + struct sa1100_port *sport =
> + container_of(port, struct sa1100_port, port);
>
> - if (sport->port.x_char) {
> - UART_PUT_CHAR(sport, sport->port.x_char);
> - sport->port.icount.tx++;
> - sport->port.x_char = 0;
> - return;
> - }
> + return UART_GET_UTSR1(sport) & UTSR1_TNF;
> +}
> +
> +static void sa1100_put_char(struct uart_port *port, unsigned char ch)
> +{
> + struct sa1100_port *sport =
> + container_of(port, struct sa1100_port, port);
> +
> + UART_PUT_CHAR(sport, ch);
> +}
Perhaps not to add into this change itself, but I just point out these
would be useful in addition to what is changed:
- Get rid of UART_PUT_CHAR()
- sa1100_console_putchar() could use sa1100_tx_ready()
> diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
> index 6f08136ce78a..ff430a1cc3a2 100644
> --- a/drivers/tty/serial/vt8500_serial.c
> +++ b/drivers/tty/serial/vt8500_serial.c
> @@ -187,37 +187,17 @@ static void handle_rx(struct uart_port *port)
> tty_flip_buffer_push(tport);
> }
>
> -static void handle_tx(struct uart_port *port)
> +static unsigned int vt8500_tx_empty(struct uart_port *port)
> {
> - struct circ_buf *xmit = &port->state->xmit;
> -
> - if (port->x_char) {
> - writeb(port->x_char, port->membase + VT8500_TXFIFO);
> - port->icount.tx++;
> - port->x_char = 0;
> - }
> - if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
> - vt8500_stop_tx(port);
> - return;
> - }
> + unsigned int idx = vt8500_read(port, VT8500_URFIDX) & 0x1f;
>
> - while ((vt8500_read(port, VT8500_URFIDX) & 0x1f) < 16) {
> - if (uart_circ_empty(xmit))
> - break;
> -
> - writeb(xmit->buf[xmit->tail], port->membase + VT8500_TXFIFO);
> -
> - xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> - port->icount.tx++;
> - }
> -
> - if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> - uart_write_wakeup(port);
> -
> - if (uart_circ_empty(xmit))
> - vt8500_stop_tx(port);
> + return idx < 16 ? TIOCSER_TEMT : 0;
> }
So there's just plain move and re-layouting of existing vt8500_tx_empty()
embedded here. Why not do it in a separate change beforehand?
I discovered this the hard way, that is, I started to look why tx on
earth "FIFO index" < 16 should be called "tx empty", if it would be a
separate change it would have been immediately obvious that it was a
pre-existing thing.
> +static DEFINE_UART_PORT_TX_HELPER(handle_tx, port, ch,
> + vt8500_tx_empty(port),
> + writeb(ch, port->membase + VT8500_TXFIFO));
> +
> static void vt8500_start_tx(struct uart_port *port)
> {
> struct vt8500_port *vt8500_port = container_of(port,
> @@ -260,12 +240,6 @@ static irqreturn_t vt8500_irq(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> -static unsigned int vt8500_tx_empty(struct uart_port *port)
> -{
> - return (vt8500_read(port, VT8500_URFIDX) & 0x1f) < 16 ?
> - TIOCSER_TEMT : 0;
> -}
> -
> static unsigned int vt8500_get_mctrl(struct uart_port *port)
> {
> unsigned int usr;
>
--
i.
