Re: [PATCH v2 2/2] drivers/firmware: consolidate EFI framebuffer setup for all arches

[Javier Martinez Canillas <javierm@redhat.com>]

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