diff mbox series

[v6,5/9] video console: move vidconsole_get_font_size() to test.h

Message ID 20230223181030.1596976-6-dsankouski@gmail.com
State Superseded
Delegated to: Anatolij Gustschin
Headers show
Series vidconsole: refactoring and support for wider fonts | expand

Commit Message

Dzmitry Sankouski Feb. 23, 2023, 6:10 p.m. UTC 
vidconsole_get_font_size is only used in tests and in font
command. It's role in 'font size' command was to only fetch
current font name, to be used in select font function.
This is redundant, because it's easy to check for empty
string, and reuse current name right in select function.

Test functions in public API use memory and clutter interface.

Move vidconsole_get_font_size to new cmd/test.h file.
Wrap it's implementation with #ifdef only when tests enabled.

Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
---
Changes for v2: N/A
Changes for v3: N/A
Charges for v4: N/A
Charges for v5: N/A
Charges for v6: N/A

 cmd/font.c                       |  5 ++---
 drivers/video/console_truetype.c |  8 +++++++-
 include/cmd/test.h               | 19 +++++++++++++++++++
 include/video_console.h          |  9 ---------
 test/cmd/font.c                  |  1 +
 5 files changed, 29 insertions(+), 13 deletions(-)
 create mode 100644 include/cmd/test.h

Comments

Simon Glass Feb. 26, 2023, 2:56 p.m. UTC | #1 
Hi Dzmitry,

On Thu, 23 Feb 2023 at 11:10, Dzmitry Sankouski <dsankouski@gmail.com> wrote:
>
> vidconsole_get_font_size is only used in tests and in font
> command. It's role in 'font size' command was to only fetch
> current font name, to be used in select font function.
> This is redundant, because it's easy to check for empty
> string, and reuse current name right in select function.
>
> Test functions in public API use memory and clutter interface.
>
> Move vidconsole_get_font_size to new cmd/test.h file.
> Wrap it's implementation with #ifdef only when tests enabled.
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
> Changes for v2: N/A
> Changes for v3: N/A
> Charges for v4: N/A
> Charges for v5: N/A
> Charges for v6: N/A
>
>  cmd/font.c                       |  5 ++---
>  drivers/video/console_truetype.c |  8 +++++++-
>  include/cmd/test.h               | 19 +++++++++++++++++++
>  include/video_console.h          |  9 ---------
>  test/cmd/font.c                  |  1 +
>  5 files changed, 29 insertions(+), 13 deletions(-)
>  create mode 100644 include/cmd/test.h

I'm not a fan of this for three reasons:

- it changes the current select_font() API, in which NULL currently
means to select the default font, not the current one (although that
seems to be broken in the code!)
- we may want to allow checking the size of a font
- don't want #ifdefs for test in console_truetype.c (tests should
ideally just work with the normal plumbing)

Regards,
Simon
diff mbox series

Patch

diff --git a/cmd/font.c b/cmd/font.c
index 7b4347f32b..af542b01ee 100644
--- a/cmd/font.c
+++ b/cmd/font.c
@@ -51,7 +51,7 @@  static int do_font_select(struct cmd_tbl *cmdtp, int flag, int argc,
 static int do_font_size(struct cmd_tbl *cmdtp, int flag, int argc,
 			char *const argv[])
 {
-	const char *font_name;
+	const char *empty_name = {'\0'};
 	struct udevice *dev;
 	uint size;
 	int ret;
@@ -61,11 +61,10 @@  static int do_font_size(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	if (uclass_first_device_err(UCLASS_VIDEO_CONSOLE, &dev))
 		return CMD_RET_FAILURE;
-	font_name = vidconsole_get_font_size(dev, &size);
 
 	size = dectoul(argv[1], NULL);
 
-	ret = vidconsole_select_font(dev, font_name, size);
+	ret = vidconsole_select_font(dev, empty_name, size);
 	if (ret) {
 		printf("Failed (error %d)\n", ret);
 		return CMD_RET_FAILURE;
diff --git a/drivers/video/console_truetype.c b/drivers/video/console_truetype.c
index 9cac9a6de4..f57d8b6dae 100644
--- a/drivers/video/console_truetype.c
+++ b/drivers/video/console_truetype.c
@@ -1,6 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0+
 /*
  * Copyright (c) 2016 Google, Inc
+ * (C) Copyright 2023 Dzmitry Sankouski <dsankouski@gmail.com>
  */
 
 #include <common.h>
@@ -9,6 +10,9 @@ 
 #include <malloc.h>
 #include <video.h>
 #include <video_console.h>
+#if IS_ENABLED(CONFIG_UT_DM)
+#include <cmd/test.h>
+#endif
 
 /* Functions needed by stb_truetype.h */
 static int tt_floor(double val)
@@ -691,7 +695,7 @@  static int truetype_select_font(struct udevice *dev, const char *name,
 		if (!size)
 			size = CONFIG_CONSOLE_TRUETYPE_SIZE;
 		if (!name)
-			name = font_table->name;
+			name = priv->cur_met->font_name;
 
 		met = find_metrics(dev, name, size);
 		if (!met) {
@@ -724,6 +728,7 @@  static int truetype_select_font(struct udevice *dev, const char *name,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_UT_DM)
 const char *vidconsole_get_font_size(struct udevice *dev, uint *sizep)
 {
 	struct console_tt_priv *priv = dev_get_priv(dev);
@@ -733,6 +738,7 @@  const char *vidconsole_get_font_size(struct udevice *dev, uint *sizep)
 
 	return met->font_name;
 }
+#endif
 
 static int console_truetype_probe(struct udevice *dev)
 {
diff --git a/include/cmd/test.h b/include/cmd/test.h
new file mode 100644
index 0000000000..39afb99b73
--- /dev/null
+++ b/include/cmd/test.h
@@ -0,0 +1,19 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2015 Google, Inc
+ * (C) Copyright 2023 Dzmitry Sankouski <dsankouski@gmail.com>
+ */
+
+#ifndef __cmd_test_h
+#define __cmd_test_h
+
+/**
+ * vidconsole_get_font_size() - get the current font name and size
+ *
+ * @dev: vidconsole device
+ * @sizep: Place to put the font size (nominal height in pixels)
+ * Returns: Current font name
+ */
+const char *vidconsole_get_font_size(struct udevice *dev, uint *sizep);
+
+#endif
diff --git a/include/video_console.h b/include/video_console.h
index 9d2c0f210e..9c3d0ad305 100644
--- a/include/video_console.h
+++ b/include/video_console.h
@@ -298,15 +298,6 @@  void vidconsole_set_cursor_pos(struct udevice *dev, int x, int y);
  */
 void vidconsole_list_fonts(struct udevice *dev);
 
-/**
- * vidconsole_get_font_size() - get the current font name and size
- *
- * @dev: vidconsole device
- * @sizep: Place to put the font size (nominal height in pixels)
- * Returns: Current font name
- */
-const char *vidconsole_get_font_size(struct udevice *dev, uint *sizep);
-
 #ifdef CONFIG_VIDEO_COPY
 /**
  * vidconsole_sync_copy() - Sync back to the copy framebuffer
diff --git a/test/cmd/font.c b/test/cmd/font.c
index adb353965a..524004bed8 100644
--- a/test/cmd/font.c
+++ b/test/cmd/font.c
@@ -11,6 +11,7 @@ 
 #include <video_console.h>
 #include <test/suites.h>
 #include <test/ut.h>
+#include <cmd/test.h>
 
 /* Declare a new fdt test */
 #define FONT_TEST(_name, _flags)	UNIT_TEST(_name, _flags, font_test)