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

[Thomas Zimmermann <tzimmermann@suse.de>]

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.

Best regards
Thomas

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