Re: [PATCH 10/36] tty/vt: consolemap: introduce UNI_*() macros
From: Jiri Slaby
Date: Wed Jun 08 2022 - 03:59:23 EST
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