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

[Sam Ravnborg <sam@ravnborg.org>]

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 <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