Hello Thomas,
Thanks a lot for your feedback!
On 6/23/21 12:06 PM, Thomas Zimmermann wrote:
[snip]
>> config SYSFB
>> bool
>> default y
>> - depends on X86 || COMPILE_TEST
>> + depends on X86 || ARM || ARM64 || RISCV || COMPILE_TEST
>>
>> -config X86_SYSFB
>> +config SYSFB_SIMPLEFB
>
> You should also update the help text for simpledrm to reflect this
> change. See drivers/gpu/drm/tiny/Kconfig.
>
Indeed, I missed that. I'll update it in v3.
[snip]
>> +
>> +__init void sysfb_apply_efi_quirks(struct platform_device *pd)
>> {
>> if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI ||
>> !(screen_info.capabilities & VIDEO_CAPABILITY_SKIP_QUIRKS))
>> @@ -281,4 +348,10 @@ __init void sysfb_apply_efi_quirks(void)
>> screen_info.lfb_height = temp;
>> screen_info.lfb_linelength = 4 * screen_info.lfb_width;
>> }
>> +
>> + if (screen_info.orig_video_isVGA == VIDEO_TYPE_EFI &&
>> + IS_ENABLED(CONFIG_PCI)) {
>
> We have a 100-character limit now. This should (?) fit onto a single line.
>
>
Oh, I didn't know the character limit was extended to 100 chars now.
[snip]
>>
>> /* if the FB is incompatible, create a legacy framebuffer device */
>> if (si->orig_video_isVGA == VIDEO_TYPE_EFI)
>> - name = "efi-framebuffer";
>> + pd->name = "efi-framebuffer";
>> else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
>> - name = "vesa-framebuffer";
>> + pd->name = "vesa-framebuffer";
>> else
>> - name = "platform-framebuffer";
>> + pd->name = "platform-framebuffer";
>
> You're allocating the platform device with an empty name string. And
> here you're overwriting the pointer. Can you rearrange the code to first
> detect a proper name and then allocate the device with that name? It
> takes a few bytes more memory, but seems in the spirit of the interface.
>
Right, I'll change that in v3 as well.
> Best regards
> Thomas
>
Best regards,
--
Javier Martinez Canillas
Software Engineer
New Platform Technologies Enablement team
RHEL Engineering
