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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

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)