Hi
Am 08.09.25 um 20:51 schrieb Sam Ravnborg:
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:
Hi Thomas.
On Mon, Aug 18, 2025 at 12:36:39PM +0200, Thomas Zimmermann wrote:
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 <[email protected]>
---
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.
Thanks a lot. I'll update the series and keep it around for a bit, in
case anyone else wants to comment.
Best regards
Thomas
I look forward to see the next batches of refactoring you have planned.
Sam
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
