tty/vt: Cleanups for keyboard driver

PATCH Superseded Alexey Gladkov 2 participants 11 messages Feb 18, 2025 – Feb 21, 2025 TTY layer and serial drivers

Share & tools

Alexey Gladkov

author

2025-02-18 12:30 UTC #0 lore raw

cover tty/vt: Cleanups for keyboard driver

This is a few minor cleanups I found in keybord driver. There's nothing important here and these changes just slightly improve readability.

Alexey Gladkov (2):
tty/vt: Use KVAL instead of use bit operation tty/vt: Gather the code that outputs char with utf8 in mind

 drivers/tty/vt/keyboard.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

Signature · 1 line

2.48.1

Alexey Gladkov

author

2025-02-18 12:30 UTC #1 lore raw

PATCH 1/2 tty/vt: Use KVAL instead of use bit operation

The K_HANDLERS always gets KVAL as an argument. It is better to use the KVAL macro itself instead of bit operation.

Signed-off-by Alexey Gladkov <[email protected]>

---
 drivers/tty/vt/keyboard.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/tty/vt/keyboard.c +2 -2

index 804355da46f5..7df041ac4d5c 100644
@@ -885,7 +885,7 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
 		if (kbd->kbdmode == VC_UNICODE)
 			to_utf8(vc, npadch_value);
 		else
-			put_queue(vc, npadch_value & 0xff);
+			put_queue(vc, KVAL(npadch_value));
 		npadch_active = false;
 	}
 }

@@ -1519,7 +1519,7 @@ static void kbd_keycode(unsigned int keycode, int down, bool hw_raw)
 	if ((raw_mode || kbd->kbdmode == VC_OFF) && type != KT_SPEC && type != KT_SHIFT)
 		return;
 
-	(*k_handler[type])(vc, keysym & 0xff, !down);
+	(*k_handler[type])(vc, KVAL(keysym), !down);
 
 	param.ledstate = kbd->ledflagstate;
 	atomic_notifier_call_chain(&keyboard_notifier_list, KBD_POST_KEYSYM, &param);

Signature · 1 line

2.48.1

Alexey Gladkov

author

2025-02-18 12:30 UTC #6 lore raw

PATCH 2/2 tty/vt: Gather the code that outputs char with utf8 in mind

In reply to Alexey Gladkov

When we putting character to the tty, we take into account the keyboard mode to properly handle utf8. This code is duplicated few times.

Signed-off-by Alexey Gladkov <[email protected]>

---
 drivers/tty/vt/keyboard.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

drivers/tty/vt/keyboard.c +14 ~~-21~~

index 7df041ac4d5c..eb32bf00fe7b 100644
@@ -376,6 +376,17 @@ static void to_utf8(struct vc_data *vc, uint c)
 	}
 }
 
+static void put_queue_utf8(struct vc_data *vc, u32 value)
+{
+	if (kbd->kbdmode == VC_UNICODE)
+		to_utf8(vc, value);
+	else {
+		int c = conv_uni_to_8bit(value);
+		if (c != -1)
+			put_queue(vc, c);
+	}
+}
+
 /* FIXME: review locking for vt.c callers */
 static void set_leds(void)
 {

@@ -454,13 +465,7 @@ static unsigned int handle_diacr(struct vc_data *vc, unsigned int ch)
 	if (ch == ' ' || ch == (BRL_UC_ROW|0) || ch == d)
 		return d;
 
-	if (kbd->kbdmode == VC_UNICODE)
-		to_utf8(vc, d);
-	else {
-		int c = conv_uni_to_8bit(d);
-		if (c != -1)
-			put_queue(vc, c);
-	}
+	put_queue_utf8(vc, d);
 
 	return ch;
 }

@@ -471,13 +476,7 @@ static unsigned int handle_diacr(struct vc_data *vc, unsigned int ch)
 static void fn_enter(struct vc_data *vc)
 {
 	if (diacr) {
-		if (kbd->kbdmode == VC_UNICODE)
-			to_utf8(vc, diacr);
-		else {
-			int c = conv_uni_to_8bit(diacr);
-			if (c != -1)
-				put_queue(vc, c);
-		}
+		put_queue_utf8(vc, diacr);
 		diacr = 0;
 	}

@@ -685,13 +684,7 @@ static void k_unicode(struct vc_data *vc, unsigned int value, char up_flag)
 		diacr = value;
 		return;
 	}
-	if (kbd->kbdmode == VC_UNICODE)
-		to_utf8(vc, value);
-	else {
-		int c = conv_uni_to_8bit(value);
-		if (c != -1)
-			put_queue(vc, c);
-	}
+	put_queue_utf8(vc, value);
 }
 
 /*

Signature · 1 line

2.48.1

Jiri Slaby

maintainer

2025-02-19 06:24 UTC #2 lore raw

Re: [PATCH v1 1/2] tty/vt: Use KVAL instead of use bit operation

In reply to Alexey Gladkov

Quoted 13 lines · Alexey Gladkov

The K_HANDLERS always gets KVAL as an argument. It is better to use the KVAL macro itself instead of bit operation.
Signed-off-by Alexey Gladkov <[email protected]>
---
  drivers/tty/vt/keyboard.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/vt/keyboard.c +0 -0
index 804355da46f5..7df041ac4d5c 100644

drivers/tty/vt/keyboard.c static void k_shift(struct vc_data *vc, unsigned char value, char up_flag) · line 888

Quoted 5 lines

drivers/tty/vt/keyboard.c +1 -1

@@ -885,7 +885,7 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
  		if (kbd->kbdmode == VC_UNICODE)
  			to_utf8(vc, npadch_value);
  		else
-			put_queue(vc, npadch_value & 0xff);
+			put_queue(vc, KVAL(npadch_value));

While the mask is the same, this is not a kval, right?

drivers/tty/vt/keyboard.c static void kbd_keycode(unsigned int keycode, int down, bool hw_raw) · line 1522

Quoted 9 lines

drivers/tty/vt/keyboard.c +1 -1

  		npadch_active = false;
  	}
  }

@@ -1519,7 +1519,7 @@ static void kbd_keycode(unsigned int keycode, int down, bool hw_raw)
  	if ((raw_mode || kbd->kbdmode == VC_OFF) && type != KT_SPEC && type != KT_SHIFT)
  		return;
 
-	(*k_handler[type])(vc, keysym & 0xff, !down);
+	(*k_handler[type])(vc, KVAL(keysym), !down);

This makes sense.

Signature · 2 lines

js
suse labs

Jiri Slaby

maintainer Reviewed-by on v1 2/2

2025-02-19 06:26 UTC #7 lore raw

Re: [PATCH v1 2/2] tty/vt: Gather the code that outputs char with utf8 in mind

In reply to Alexey Gladkov

Quoted 4 lines · Alexey Gladkov

When we putting character to the tty, we take into account the keyboard mode to properly handle utf8. This code is duplicated few times.
Signed-off-by Alexey Gladkov <[email protected]>

Reviewed-by Jiri Slaby <[email protected]>

thanks,

Signature · 2 lines

js
suse labs

Alexey Gladkov

author

2025-02-19 09:23 UTC #3 lore raw

Re: [PATCH v1 1/2] tty/vt: Use KVAL instead of use bit operation

In reply to Jiri Slaby

Quoted 9 lines · Jiri Slaby

Quoted 8 lines · Alexey Gladkov
The K_HANDLERS always gets KVAL as an argument. It is better to use the KVAL macro itself instead of bit operation.
Signed-off-by Alexey Gladkov <[email protected]>
---
  drivers/tty/vt/keyboard.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

Quoted 12 lines

drivers/tty/vt/keyboard.c static void k_shift(struct vc_data *vc, unsigned char value, char up_flag) · line 888
Quoted 10 lines
drivers/tty/vt/keyboard.c +1 -1
index 804355da46f5..7df041ac4d5c 100644
@@ -885,7 +885,7 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
  		if (kbd->kbdmode == VC_UNICODE)
  			to_utf8(vc, npadch_value);
  		else
-			put_queue(vc, npadch_value & 0xff);
+			put_queue(vc, KVAL(npadch_value));
While the mask is the same, this is not a kval, right?

I'm pretty sure it's KVAL, but to be honest I don't understand why it is not done for to_utf8() as well. All values passed to to_utf8() must be kval.

We call to_utf8() in k_unicode, fn_enter (through k_spec), handle_diacr (through k_deadunicode or k_unicode). All K_HANDLERS take KVAL as value.

If I understand this code correctly, it is more correct to write it like this:

drivers/tty/vt/keyboard.c +3 -2

@@ -882,10 +882,11 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
 
        /* kludge */
        if (up_flag && shift_state != old_state && npadch_active) {
+               u32 kval = KVAL(npadch_value);
                if (kbd->kbdmode == VC_UNICODE)
-                       to_utf8(vc, npadch_value);
+                       to_utf8(vc, kval);
                else
-                       put_queue(vc, npadch_value & 0xff);
+                       put_queue(vc, kval);
                npadch_active = false;
        }
 }

But I may be wrong because the code about npadch_value is very old and I may be missing something.

Quoted 16 lines

drivers/tty/vt/keyboard.c static void kbd_keycode(unsigned int keycode, int down, bool hw_raw) · line 1522
Quoted 9 lines
drivers/tty/vt/keyboard.c +1 -1
  		npadch_active = false;
  	}
  }
@@ -1519,7 +1519,7 @@ static void kbd_keycode(unsigned int keycode, int down, bool hw_raw)
  	if ((raw_mode || kbd->kbdmode == VC_OFF) && type != KT_SPEC && type != KT_SHIFT)
  		return;
 
-	(*k_handler[type])(vc, keysym & 0xff, !down);
+	(*k_handler[type])(vc, KVAL(keysym), !down);
This makes sense.
Signature · 2 lines
js
suse labs

Signature · 1 line

Rgrds, legion

Jiri Slaby

maintainer

2025-02-19 09:33 UTC #4 lore raw

Re: [PATCH v1 1/2] tty/vt: Use KVAL instead of use bit operation

Quoted 23 lines · Alexey Gladkov

Quoted 9 lines · Jiri Slaby
Quoted 8 lines · Alexey Gladkov
The K_HANDLERS always gets KVAL as an argument. It is better to use the KVAL macro itself instead of bit operation.
Signed-off-by Alexey Gladkov <[email protected]>
---
   drivers/tty/vt/keyboard.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 12 lines
drivers/tty/vt/keyboard.c static void k_shift(struct vc_data *vc, unsigned char value, char up_flag) · line 888
Quoted 10 lines
drivers/tty/vt/keyboard.c +1 -1
index 804355da46f5..7df041ac4d5c 100644
@@ -885,7 +885,7 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
   		if (kbd->kbdmode == VC_UNICODE)
   			to_utf8(vc, npadch_value);
   		else
-			put_queue(vc, npadch_value & 0xff);
+			put_queue(vc, KVAL(npadch_value));
While the mask is the same, this is not a kval, right?

Quoted 3 lines

I'm pretty sure it's KVAL, but to be honest I don't understand why it is not done for to_utf8() as well. All values passed to to_utf8() must be kval.

Not at all, it handles multibyte chars.

Quoted 2 lines

We call to_utf8() in k_unicode, fn_enter (through k_spec), handle_diacr (through k_deadunicode or k_unicode). All K_HANDLERS take KVAL as value.

Yes, but pass unicode multibyte to to_utf8().

drivers/tty/vt/keyboard.c static void k_shift(struct vc_data *vc, unsigned char value, char up_flag) · line 887

Quoted 13 lines

If I understand this code correctly, it is more correct to write it like this:

drivers/tty/vt/keyboard.c +2 -1

@@ -882,10 +882,11 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
 
         /* kludge */
         if (up_flag && shift_state != old_state && npadch_active) {
+               u32 kval = KVAL(npadch_value);
                 if (kbd->kbdmode == VC_UNICODE)
-                       to_utf8(vc, npadch_value);
+                       to_utf8(vc, kval);

Definitely not, as you want to pass that multibyte char in.

Signature · 2 lines

js
suse labs

Alexey Gladkov

author

2025-02-19 11:53 UTC #5 lore raw

Re: [PATCH v1 1/2] tty/vt: Use KVAL instead of use bit operation

Quoted 42 lines · Jiri Slaby

Quoted 23 lines · Alexey Gladkov
Quoted 9 lines · Jiri Slaby
Quoted 8 lines · Alexey Gladkov
The K_HANDLERS always gets KVAL as an argument. It is better to use the KVAL macro itself instead of bit operation.
Signed-off-by Alexey Gladkov <[email protected]>
---
   drivers/tty/vt/keyboard.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 12 lines
drivers/tty/vt/keyboard.c static void k_shift(struct vc_data *vc, unsigned char value, char up_flag) · line 888
Quoted 10 lines
drivers/tty/vt/keyboard.c +1 -1
index 804355da46f5..7df041ac4d5c 100644
@@ -885,7 +885,7 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
   		if (kbd->kbdmode == VC_UNICODE)
   			to_utf8(vc, npadch_value);
   		else
-			put_queue(vc, npadch_value & 0xff);
+			put_queue(vc, KVAL(npadch_value));
While the mask is the same, this is not a kval, right?
Quoted 3 lines
I'm pretty sure it's KVAL, but to be honest I don't understand why it is not done for to_utf8() as well. All values passed to to_utf8() must be kval.
Not at all, it handles multibyte chars.
Quoted 2 lines
We call to_utf8() in k_unicode, fn_enter (through k_spec), handle_diacr (through k_deadunicode or k_unicode). All K_HANDLERS take KVAL as value.
Yes, but pass unicode multibyte to to_utf8().
Quoted 7 lines
If I understand this code correctly, it is more correct to write it like this:
drivers/tty/vt/keyboard.c +0 -0
@@ -882,10 +882,11 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
 

Quoted 8 lines

drivers/tty/vt/keyboard.c static void k_shift(struct vc_data *vc, unsigned char value, char up_flag) · line 887

Quoted 6 lines

drivers/tty/vt/keyboard.c +2 -1

         /* kludge */
         if (up_flag && shift_state != old_state && npadch_active) {
+               u32 kval = KVAL(npadch_value);
                 if (kbd->kbdmode == VC_UNICODE)
-                       to_utf8(vc, npadch_value);
+                       to_utf8(vc, kval);

Definitely not, as you want to pass that multibyte char in.

Ok. So I misunderstood this code. I will remove this change from the next version.

Signature · 1 line

Rgrds, legion

Alexey Gladkov

author

2025-02-21 12:44 UTC #8 lore raw

v2 cover tty/vt: Cleanups for keyboard driver

In reply to Alexey Gladkov

This is a few minor cleanups I found in keybord driver. There's nothing important here and these changes just slightly improve readability.

---
v2:
* Removed the faulty use of KVAL for npadch_value.

Alexey Gladkov (2):
tty/vt: Use KVAL instead of use bit operation tty/vt: Gather the code that outputs char with utf8 in mind

 drivers/tty/vt/keyboard.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

Signature · 1 line

2.48.1

Alexey Gladkov

author

2025-02-21 12:44 UTC #9 lore raw

PATCH v2 1/2 tty/vt: Use KVAL instead of use bit operation

The K_HANDLERS always gets KVAL as an argument. It is better to use the KVAL macro itself instead of bit operation.

Signed-off-by Alexey Gladkov <[email protected]>

---
 drivers/tty/vt/keyboard.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/tty/vt/keyboard.c +1 -1

index 804355da46f5..38d4df932f73 100644
@@ -1519,7 +1519,7 @@ static void kbd_keycode(unsigned int keycode, int down, bool hw_raw)
 	if ((raw_mode || kbd->kbdmode == VC_OFF) && type != KT_SPEC && type != KT_SHIFT)
 		return;
 
-	(*k_handler[type])(vc, keysym & 0xff, !down);
+	(*k_handler[type])(vc, KVAL(keysym), !down);
 
 	param.ledstate = kbd->ledflagstate;
 	atomic_notifier_call_chain(&keyboard_notifier_list, KBD_POST_KEYSYM, &param);

Signature · 1 line

2.48.1

Alexey Gladkov

author

2025-02-21 12:44 UTC #10 lore raw

PATCH v2 2/2 tty/vt: Gather the code that outputs char with utf8 in mind

In reply to Alexey Gladkov

When we putting character to the tty, we take into account the keyboard mode to properly handle utf8. This code is duplicated few times.

Signed-off-by Alexey Gladkov <[email protected]>

Reviewed-by Jiri Slaby <[email protected]>

---
 drivers/tty/vt/keyboard.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

drivers/tty/vt/keyboard.c +14 ~~-21~~

index 38d4df932f73..517db498beed 100644
@@ -376,6 +376,17 @@ static void to_utf8(struct vc_data *vc, uint c)
 	}
 }
 
+static void put_queue_utf8(struct vc_data *vc, u32 value)
+{
+	if (kbd->kbdmode == VC_UNICODE)
+		to_utf8(vc, value);
+	else {
+		int c = conv_uni_to_8bit(value);
+		if (c != -1)
+			put_queue(vc, c);
+	}
+}
+
 /* FIXME: review locking for vt.c callers */
 static void set_leds(void)
 {

@@ -454,13 +465,7 @@ static unsigned int handle_diacr(struct vc_data *vc, unsigned int ch)
 	if (ch == ' ' || ch == (BRL_UC_ROW|0) || ch == d)
 		return d;
 
-	if (kbd->kbdmode == VC_UNICODE)
-		to_utf8(vc, d);
-	else {
-		int c = conv_uni_to_8bit(d);
-		if (c != -1)
-			put_queue(vc, c);
-	}
+	put_queue_utf8(vc, d);
 
 	return ch;
 }

@@ -471,13 +476,7 @@ static unsigned int handle_diacr(struct vc_data *vc, unsigned int ch)
 static void fn_enter(struct vc_data *vc)
 {
 	if (diacr) {
-		if (kbd->kbdmode == VC_UNICODE)
-			to_utf8(vc, diacr);
-		else {
-			int c = conv_uni_to_8bit(diacr);
-			if (c != -1)
-				put_queue(vc, c);
-		}
+		put_queue_utf8(vc, diacr);
 		diacr = 0;
 	}

@@ -685,13 +684,7 @@ static void k_unicode(struct vc_data *vc, unsigned int value, char up_flag)
 		diacr = value;
 		return;
 	}
-	if (kbd->kbdmode == VC_UNICODE)
-		to_utf8(vc, value);
-	else {
-		int c = conv_uni_to_8bit(value);
-		if (c != -1)
-			put_queue(vc, c);
-	}
+	put_queue_utf8(vc, value);
 }
 
 /*

Signature · 1 line

2.48.1