Re: [PATCH 00/47] fbdev: Use I/O helpers

[Sam Ravnborg <sam@ravnborg.org>]

Hi Helge,

On Fri, Jul 28, 2023 at 08:46:59PM +0200, Helge Deller wrote:
> On 7/28/23 18:39, Thomas Zimmermann wrote:
> > Most fbdev drivers operate on I/O memory.
> 
> Just nitpicking here:
> What is I/O memory?
> Isn't it either memory, or I/O ?
> I mean, I would never think of the cfb* draw functions under I/O.
> 
> > And most of those use the
> > default implementations for file I/O and console drawing. Convert all
> > these low-hanging fruits to the fb_ops initializer macro and Kconfig
> > token for fbdev I/O helpers.
> 
> I do see the motivation for your patch, but I think the
> macro names are very misleading.
> 
> You have:
> #define __FB_DEFAULT_IO_OPS_RDWR \
>         .fb_read        = fb_io_read, \
>         .fb_write       = fb_io_write
> 
> #define __FB_DEFAULT_IO_OPS_DRAW \
>         .fb_fillrect    = cfb_fillrect, \
>         .fb_copyarea    = cfb_copyarea, \
>         .fb_imageblit   = cfb_imageblit
> 
> #define __FB_DEFAULT_IO_OPS_MMAP \
>         .fb_mmap        = NULL /* default implementation */
> 
> #define FB_DEFAULT_IO_OPS \
>         __FB_DEFAULT_IO_OPS_RDWR, \
>         __FB_DEFAULT_IO_OPS_DRAW, \
>         __FB_DEFAULT_IO_OPS_MMAP
> 
> I think FB_DEFAULT_IO_OPS is OK for read/write/mmap.
> But I would suggest to split out __FB_DEFAULT_IO_OPS_DRAW.
> Something like:
> #define FB_DEFAULT_IO_OPS \
>         __FB_DEFAULT_IO_OPS_RDWR, \
>         __FB_DEFAULT_IO_OPS_MMAP


> #define FB_DEFAULT_CFB_OPS \
>         .fb_fillrect    = cfb_fillrect, \
>         .fb_copyarea    = cfb_copyarea, \
>         .fb_imageblit   = cfb_imageblit

The prefix cfb, I have recently learned, equals color frame buffer.
They are named such for purely historical reasons.

What is important is where the data are copied as we have two
implementations of for example copyarea - one using system memory, the
other using IO memory.

The naming FB_DEFAULT_IO_OPS says this is the defaults to IO memory
operations, which tell what they do and avoid the strange cfb acronym.

Reserve cfb for color frame buffers only - and maybe in the end rename
the three cfbcopyarea, cfbfillrect, cfbimgblt to use the io prefix.
Which is much simpler to do after this series - and nice extra benefit.

I hope this properly explains why I like the current naming and
acked it when the macros were introduced.

	Sam