Re: [PATCH v2 2/3] tty: serial: use DEFINE_UART_PORT_TX_HELPER()

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

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

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@distanz.ch>
> Cc: Richard Genoud <richard.genoud@gmail.com>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: Vladimir Zapolskiy <vz@mleia.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: "Andreas Färber" <afaerber@suse.de>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---

> 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.