Re: [PATCH 10/36] tty/vt: consolemap: introduce UNI_*() macros

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 08. 06. 22, 8:59, Jiri Slaby wrote:
On 07. 06. 22, 15:47, Ilpo Järvinen wrote:
On Tue, 7 Jun 2022, Jiri Slaby wrote:

The code currently does shift, OR, and AND logic directly in the code.
It is not much obvious what happens there. Therefore define four macros
for that purpose and use them in the code. We use GENMASK() so that it
is clear which bits serve what purpose:
- UNI_GLYPH: bits  0.. 5
- UNI_ROW:   bits  6..10
- UNI_DIR:   bits 11..31

Signed-off-by: Jiri Slaby <jslaby@xxxxxxx>
---
  drivers/tty/vt/consolemap.c | 21 +++++++++++++--------
  1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index 016c1a0b4290..e5fd225e87bd 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -190,6 +190,11 @@ static int inv_translate[MAX_NR_CONSOLES];
  #define UNI_DIR_ROWS    32U
  #define UNI_ROW_GLYPHS    64U
+#define UNI_DIR(uni)        ( (uni)                   >> 11)
+#define UNI_ROW(uni)        (((uni) & GENMASK(10, 6)) >>  6)

This is opencoding what FIELD_GET() does. Maybe just define these as
masks and use FIELD_GET in the code below.

Ah, great -- I was thinking there should be something for that purpose already, but didn't find this. But let's define these UNI_* macros using appropriate FIELD_GET(). (And not using FIELD_GET() in the code.)

+#define UNI_GLYPH(uni)        ( (uni) & GENMASK( 5, 0))
thanks,

JFYI, I ended up with this diff to the original approach:
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -23,6 +23,8 @@
  * stack overflow.
  */

+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/module.h>
 #include <linux/kd.h>
 #include <linux/errno.h>
@@ -190,10 +192,17 @@ static int inv_translate[MAX_NR_CONSOLES];
 #define UNI_DIR_ROWS   32U
 #define UNI_ROW_GLYPHS 64U

-#define UNI_DIR(uni)           ( (uni)                   >> 11)
-#define UNI_ROW(uni)           (((uni) & GENMASK(10, 6)) >>  6)
-#define UNI_GLYPH(uni)         ( (uni) & GENMASK( 5, 0))
-#define UNI(dir, row, glyph)   (((dir) << 11) | ((row) << 6) | (glyph))
+#define UNI_DIR_BITS(max)      GENMASK((max), 11)
+#define UNI_ROW_BITS           GENMASK(10,  6)
+#define UNI_GLYPH_BITS         GENMASK( 5,  0)
+
+#define UNI_DIR(uni)   FIELD_GET(UNI_DIR_BITS(sizeof(uni) * 8 - 1), (uni))
+#define UNI_ROW(uni)   FIELD_GET(UNI_ROW_BITS, (uni))
+#define UNI_GLYPH(uni) FIELD_GET(UNI_GLYPH_BITS, (uni))
+
+#define UNI(dir, row, glyph)   (FIELD_PREP(UNI_DIR_BITS(31), (dir)) | \
+                                FIELD_PREP(UNI_ROW_BITS, (row)) | \
+                                FIELD_PREP(UNI_GLYPH_BITS, (glyph)))

 /**
  * struct uni_pagedict -- unicode directory

=======================================================

More text, but easier to follow, I think. except the UNI_DIR_BITS() has to have a parameter, otherwise compilation raises a too-big value warning with use of UNI_DIR() in con_insert_unipair() where uni is only of ushort type. Alternatively, we can cast uni to u32, but that produces worse assembly (extensions to u32 here and there).

thanks,
--
js
suse labs



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux