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

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

and then add FB_DEFAULT_IO_OPS *and* FB_DEFAULT_CFB_OPS
to the various struct fb_ops in the drivers.

Helge