| Message ID | 01ee8849ef8dc49c93a77bc4961ad56b9d435b8a.1739881707.git.legion@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | tty/vt: Cleanups for keyboard driver | expand |
On 18. 02. 25, 13:29, Alexey Gladkov wrote: > 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 <legion@kernel.org> > --- > drivers/tty/vt/keyboard.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c > index 804355da46f5..7df041ac4d5c 100644 > --- a/drivers/tty/vt/keyboard.c > +++ b/drivers/tty/vt/keyboard.c > @@ -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? > 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.
On Wed, Feb 19, 2025 at 07:24:52AM +0100, Jiri Slaby wrote: > On 18. 02. 25, 13:29, Alexey Gladkov wrote: > > 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 <legion@kernel.org> > > --- > > drivers/tty/vt/keyboard.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c > > index 804355da46f5..7df041ac4d5c 100644 > > --- a/drivers/tty/vt/keyboard.c > > +++ b/drivers/tty/vt/keyboard.c > > @@ -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: --- a/drivers/tty/vt/keyboard.c +++ b/drivers/tty/vt/keyboard.c @@ -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. > > 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. > > -- > js > suse labs >
On 19. 02. 25, 10:23, Alexey Gladkov wrote: > On Wed, Feb 19, 2025 at 07:24:52AM +0100, Jiri Slaby wrote: >> On 18. 02. 25, 13:29, Alexey Gladkov wrote: >>> 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 <legion@kernel.org> >>> --- >>> drivers/tty/vt/keyboard.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c >>> index 804355da46f5..7df041ac4d5c 100644 >>> --- a/drivers/tty/vt/keyboard.c >>> +++ b/drivers/tty/vt/keyboard.c >>> @@ -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. Not at all, it handles multibyte chars. > 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(). > If I understand this code correctly, it is more correct to write it like > this: > > --- a/drivers/tty/vt/keyboard.c > +++ b/drivers/tty/vt/keyboard.c > @@ -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.
On Wed, Feb 19, 2025 at 10:33:38AM +0100, Jiri Slaby wrote: > On 19. 02. 25, 10:23, Alexey Gladkov wrote: > > On Wed, Feb 19, 2025 at 07:24:52AM +0100, Jiri Slaby wrote: > >> On 18. 02. 25, 13:29, Alexey Gladkov wrote: > >>> 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 <legion@kernel.org> > >>> --- > >>> drivers/tty/vt/keyboard.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c > >>> index 804355da46f5..7df041ac4d5c 100644 > >>> --- a/drivers/tty/vt/keyboard.c > >>> +++ b/drivers/tty/vt/keyboard.c > >>> @@ -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. > > Not at all, it handles multibyte chars. > > > 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(). > > > If I understand this code correctly, it is more correct to write it like > > this: > > > > --- a/drivers/tty/vt/keyboard.c > > +++ b/drivers/tty/vt/keyboard.c > > @@ -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. Ok. So I misunderstood this code. I will remove this change from the next version.
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c index 804355da46f5..7df041ac4d5c 100644 --- a/drivers/tty/vt/keyboard.c +++ b/drivers/tty/vt/keyboard.c @@ -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, ¶m);
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 <legion@kernel.org> --- drivers/tty/vt/keyboard.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)