Re: [PATCH v2 34/35] tty: make tty_get_byte_size available

[Johan Hovold <johan@kernel.org>]

On Mon, May 10, 2021 at 09:00:54AM +0200, Jiri Slaby wrote:
> Many tty drivers contain code to compute bits count depending on termios
> cflags. So extract this code from serial core to a separate tty helper
> function called tty_get_byte_size.
> 
> In the next patch, call to this new function will replace many copies of
> this code.
> 
> [v2] simplified the code flow as suggested by Joe and Andy
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Joe Perches <joe@perches.com>
> ---
>  drivers/tty/serial/serial_core.c | 30 +++--------------------
>  drivers/tty/tty_ioctl.c          | 42 ++++++++++++++++++++++++++++++++
>  include/linux/tty.h              |  2 ++
>  3 files changed, 47 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index d29329eb52f4..b3fc2b02a705 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -334,39 +334,15 @@ void
>  uart_update_timeout(struct uart_port *port, unsigned int cflag,
>  		    unsigned int baud)
>  {
> -	unsigned int bits;
> +	unsigned int size;
>  
> -	/* byte size and parity */
> -	switch (cflag & CSIZE) {
> -	case CS5:
> -		bits = 7;
> -		break;
> -	case CS6:
> -		bits = 8;
> -		break;
> -	case CS7:
> -		bits = 9;
> -		break;
> -	default:
> -		bits = 10;
> -		break; /* CS8 */
> -	}
> -
> -	if (cflag & CSTOPB)
> -		bits++;
> -	if (cflag & PARENB)
> -		bits++;
> -
> -	/*
> -	 * The total number of bits to be transmitted in the fifo.
> -	 */
> -	bits = bits * port->fifosize;
> +	size = tty_get_byte_size(cflag, true) * port->fifosize;
>
>  	/*
>  	 * Figure the timeout to send the above number of bits.
>  	 * Add .02 seconds of slop
>  	 */
> -	port->timeout = (HZ * bits) / baud + HZ/50;
> +	port->timeout = (HZ * size) / baud + HZ/50;
>  }
>  
>  EXPORT_SYMBOL(uart_update_timeout);
> diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> index aa9ecc8be990..13acc3decd87 100644
> --- a/drivers/tty/tty_ioctl.c
> +++ b/drivers/tty/tty_ioctl.c
> @@ -300,6 +300,48 @@ int tty_termios_hw_change(const struct ktermios *a, const struct ktermios *b)
>  }
>  EXPORT_SYMBOL(tty_termios_hw_change);
>  
> +/**
> + *	tty_get_byte_size	-	get size of a byte
> + *	@cflag: termios cflag value
> + *	@account_flags: account for start and stop bits, second stop bit (if
> + *			set), and parity (if set)
> + *
> + *	Get the size of a byte in bits depending on @cflag. Depending on
> + *	@account_flags parameter, the result also accounts start and stop bits,
> + *	the second stop bit, and parity bit.
> + */
> +unsigned char tty_get_byte_size(unsigned int cflag, bool account_flags)
> +{
> +	unsigned char bits;
> +
> +	switch (cflag & CSIZE) {
> +	case CS5:
> +		bits = 5;
> +		break;
> +	case CS6:
> +		bits = 6;
> +		break;
> +	case CS7:
> +		bits = 7;
> +		break;
> +	case CS8:
> +	default:
> +		bits = 8;
> +		break;
> +	}
> +
> +	if (!account_flags)
> +		return bits;
> +
> +	if (cflag & CSTOPB)
> +		bits++;
> +	if (cflag & PARENB)
> +		bits++;
> +
> +	return bits + 2;
> +}
> +EXPORT_SYMBOL_GPL(tty_get_byte_size);

This should really be two functions rather than passing a bool argument.

I think naming them

	tty_get_word_size()

and

	tty_get_frame_size()

would be much more clear than than "byte size" + flag.

I realise that the serial-driver interface only uses a cflag argument,
but I think we should consider passing a pointer to the termios
structure instead.

Johan