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

[Johan Hovold <johan@kernel.org>]

On Thu, May 13, 2021 at 09:24:03AM +0200, Jiri Slaby wrote:
> On 10. 05. 21, 11:47, Johan Hovold wrote:
> >> --- 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)
> ...
> >> +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.
> 
> Maybe I am screwed, but word means exactly 2B here.

No, not in this context.

Some UART datasheets use "word" and "word length", while others use
"character length" or "data bits" (and variations thereof).

> So instead, I would 
> go for:
> s/word/char/ -- might be confused with C's char, 1B, or maybe not -- or
> s/word/data/ -- more generic and generally used in serial terminology.

But "data size" it not very precise.

I'd go for either

	tty_get_word_size() or tty_get_char_size(), and
	tty_get_frame_size()

or possibly

	tty_get_data_bits(), and
	tty_get_frame_bits()

Since posix and the termios interface use "CSIZE" (even if that "C" is
ambiguous) and our man pages use "character size" perhaps we should
stick with that and use:

	tty_get_char_size(), and
	tty_get_frame_size()

That should be clear enough for everyone.

> > 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.
> 
> That's impossible as termios is not always at hand. Examples are:
> pch_uart_startup -> uart_update_timeout
> sunsab_console_setup -> sunsab_convert_to_sab -> uart_update_timeout
> sunsu_kbd_ms_init -> sunsu_change_speed -> uart_update_timeout
> 
> Let me document that in the commit.

Sounds good.

Johan