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

Hello Thomas,

Thanks a lot for your feedback!

On 6/23/21 12:06 PM, Thomas Zimmermann wrote:

[snip]

quoted

  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]
 

quoted

+
+__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]

quoted

  
  	/* 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel