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

[Helge Deller]

On 7/28/23 23:01, Sam Ravnborg wrote:
> 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.

correct.

> They are named such for purely historical reasons.

well, they operate on MEMORY which represents a (color) frame buffer,
either in host memory or memory on some card on some bus.
So, the naming cfb is not historical, but even today correct.

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

sys_copyarea() and cfb_copyarea().

> The naming FB_DEFAULT_IO_OPS says this is the defaults to IO memory
> operations, which tell what they do

This is exactly what I find misleading. IO_OPS sounds that it operates
on file I/O (like file read/write), but not on iomem.

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

Again, the io prefix is what I think is misleading.
Why not name it what it really is and what is used in the kernel already, e.g.
iomem_copyarea() and sysmem_copyarea().
which would lead to
FB_DEFAULT_IOMEM_OPS and FB_DEFAULT_SYSMEM_OPS.

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

IMHO the naming isn't perfect, but that's just nitpicking.
Besides that, Thomas' patches are a nice cleanup.
So, if you want add a
Acked-by: Helge Deller <deller at gmx.de>
to the series.

Helge