Re: [PATCH 4/6] fbcon: Move fbcon callbacks into struct fbcon_bitops

Hi Thomas.

On Mon, Sep 08, 2025 at 03:06:46PM +0200, Thomas Zimmermann wrote:

Hi Sam,

thanks for doing the review.

Am 05.09.25 um 20:53 schrieb Sam Ravnborg:

quoted

Hi Thomas.

On Mon, Aug 18, 2025 at 12:36:39PM +0200, Thomas Zimmermann wrote:

quoted

Depending on rotation settings, fbcon sets different callback
functions in struct fbcon from within fbcon_set_bitops(). Declare
the callback functions in the new type struct fbcon_bitops. Then
only replace the single bitops pointer in struct fbcon.

Keeping callbacks in constant instances of struct fbcon_bitops
makes it harder to exploit the callbacks. Also makes the code slightly
easier to maintain.

For tile-based consoles, there's a separate instance of the bitops
structure.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
  drivers/video/fbdev/core/bitblit.c   | 17 ++++---
  drivers/video/fbdev/core/fbcon.c     | 67 +++++++++++++++-------------
  drivers/video/fbdev/core/fbcon.h     |  7 ++-
  drivers/video/fbdev/core/fbcon_ccw.c | 18 +++++---
  drivers/video/fbdev/core/fbcon_cw.c  | 18 +++++---
  drivers/video/fbdev/core/fbcon_ud.c  | 18 +++++---
  drivers/video/fbdev/core/tileblit.c  | 16 ++++---
  7 files changed, 94 insertions(+), 67 deletions(-)

diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index a2202cae0691..267bd1635a41 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c

@@ -384,15 +384,18 @@ static int bit_update_start(struct fb_info *info)
  	return err;
  }
+static const struct fbcon_bitops bit_fbcon_bitops = {
+	.bmove = bit_bmove,
+	.clear = bit_clear,
+	.putcs = bit_putcs,
+	.clear_margins = bit_clear_margins,
+	.cursor = bit_cursor,
+	.update_start = bit_update_start,
+};
+
  void fbcon_set_bitops(struct fbcon *confb)
  {
-	confb->bmove = bit_bmove;
-	confb->clear = bit_clear;
-	confb->putcs = bit_putcs;
-	confb->clear_margins = bit_clear_margins;
-	confb->cursor = bit_cursor;
-	confb->update_start = bit_update_start;
-	confb->rotate_font = NULL;
+	confb->bitops = &bit_fbcon_bitops;
  	if (confb->rotate)
  		fbcon_set_rotate(confb);

fbcon_set_rotate() is only used to set the correct bitops.

It would be simpler to just do

	if (confb->rotate)
		confb->bitops = fbcon_rotate_get_ops();

And rename fbcon_set_rotate() to fbcon_rotate_get_ops() and return the
pointer to the struct.

The no need to pass the struct, and it is obvious that the bitops are
overwritten.

I tried to keep the changes here to a minimum and avoided changing the
function interfaces too much.

But did you read patch 5 already? I think the cleanup you're looking for is
there. fbcon_set_rotate() will be gone. And the update bit-op selection is
contained in fbcon_set_bitops(). I guess this could be renamed to
fbcon_update_bitops() to make it clear that it updates from internal state.

Patch 5 looks good, and is again a nice cleanup.
I like that the code is now more explicit in what it does and do not
do overwrites.

Returning a pointer or adding the assignment in a helper is not a big
deal.

With or without the suggested renaming both patch 4 + 5 are r-b.

That said, I am not expert in this field, but at least you had another
pair of eyes on the changes.
I look forward to see the next batches of refactoring you have planned.

	Sam