Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
> Hi Javier
>
> Am 04.09.23 um 14:59 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
>>
>> Hello Thomas,
>>
>>> Generate callback functions for struct fb_ops with the fbdev macro
>>> FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(). Initialize struct fb_ops to
>>> the generated functions with fbdev initializer macros.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>> Cc: Steve Glendinning <steve.glendinning@xxxxxxxxxxx>
>>> ---
>>
>> The patch looks good to me, but I've a question below.
>>
>> Acked-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
>>
>>> drivers/video/fbdev/smscufx.c | 85 +++++++++--------------------------
>>> 1 file changed, 22 insertions(+), 63 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
>>
>> [...]
>>
>>> static const struct fb_ops ufx_ops = {
>>> .owner = THIS_MODULE,
>>> - .fb_read = fb_sys_read,
>>> - .fb_write = ufx_ops_write,
>>> + __FB_DEFAULT_DEFERRED_OPS_RDWR(ufx_ops),
>>> .fb_setcolreg = ufx_ops_setcolreg,
>>> - .fb_fillrect = ufx_ops_fillrect,
>>> - .fb_copyarea = ufx_ops_copyarea,
>>> - .fb_imageblit = ufx_ops_imageblit,
>>> + __FB_DEFAULT_DEFERRED_OPS_DRAW(ufx_ops),
>>> .fb_mmap = ufx_ops_mmap,
>>
>> There are no generated functions for .fb_mmap, I wonder what's the value
>> of __FB_DEFAULT_DEFERRED_OPS_MMAP() ? Maybe just removing that macro and
>> setting .fb_mmap = fb_deferred_io_mmap instead if there's no custom mmap
>> handler would be easier to read ?
>
> At least two drivers could use __FB_DEFAULT_DEFERRED_OPS_MMAP:
> picolcd-fb and hyperv_fb. At some point, we might want to set/clear
> fb_mmap depending on some Kconfig value. Having
> __FB_DEFAULT_DEFERRED_OPS_MMAP might be helpful then.
>
Got it, thanks for the explanation.
>>
>> Alternatively, __FB_DEFAULT_DEFERRED_OPS_MMAP() could still be left but
>> not taking a __prefix argument since that is not used anyways ?
>
> The driver optionally provides mmap without deferred I/O, hence the mmap
> function. That makes no sense, as these writes to the buffer would never
> make it to the device memory. But I didn't want to remove the code
> either. So I just left the existing function as-is. Usually, the
> deferred-I/O mmap is called immediately. [1]
>
Makes sense.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
