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 <tzimmerm...@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
