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

[Sam Ravnborg <sam@ravnborg.org>]

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.

	Sam