On Thu, May 13, 2021 at 09:24:03AM +0200, Jiri Slaby wrote:

On 10. 05. 21, 11:47, Johan Hovold wrote:

quoted

quoted

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

...

quoted

quoted

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

quoted

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